MNT: Drop uses of deprecated distutils #1073
Conversation
Codecov Report
@@ Coverage Diff @@ ## master #1073 +/- ## ========================================== + Coverage 91.87% 92.28% +0.41% ========================================== Files 101 100 -1 Lines 12551 12222 -329 Branches 2406 2388 -18 ========================================== - Hits 11531 11279 -252 + Misses 683 619 -64 + Partials 337 324 -13
Continue to review full report at Codecov.
|
nibabel/cifti2/parse_cifti2.py Outdated
| self.header = Cifti2Header() | ||
| self.header.version = attrs['Version'] | ||
| if LooseVersion(self.header.version) < LooseVersion('2'): | ||
| if self.header.version not in ("2", "2.0"): |
There was a problem hiding this comment.
Is that right - that these are the only possible spellings for the version >= 2? What about versions > 2? Why not use Version('2') here instead?
There was a problem hiding this comment.
Can restore rough equivalence, just unclear why to expect future versions to be parseable by the same parser.
There was a problem hiding this comment.
Do we have some reason to think that Cifti versions won't be in standard version format? Or, put another way, what's the advantage of restricting ourselves to the strings here?
There was a problem hiding this comment.
If you think it's likely that Cifti will go to some other version convention, we can just stick with the string comparison and run an error?
effigies force-pushed the mnt/drop_distutils branch from c7f4a3a to 61bee7b Compare 
The spec requires it to be "2":
Our tests explicitly check a file with "2.0" in it, and it seems reasonable to continue with that. It's unclear whether it's a good idea to support undefined version declarations like "2.1" or "3.0", but at least it's not a regression to permit them. I'm a little inclined to emit a warning like "Unknown CIFTI version {ver}; will attempt to parse as CIFTI-2". I switched to |
| Pre-release failures are related to numpy/numpy#20863. |
Notes on CIFTI version parsing: 1) The spec only permits "2", but we have found "2.0". 2) Use packaging.version.parse to allow for non-PEP440 style strings. These will always resolve as < a PEP440 version, so the check will permit strings like "2.1" and "3.0" but not "tag2.0" or "1".
effigies force-pushed the mnt/drop_distutils branch from 61bee7b to 4fee9af Compare | Merging to get tests working. If we want to bikeshed the CIFTI2 version check, we can do it in another PR. |
2 participants
CI is failing. None of these cases seemed to hinge on the exact semantics of
LooseVersion. In most cases,packaging.version.Versionworks as well. For CIFTI-2, the spec says the string must be "2", so let's not parse that unless tests show we've been explicitly trying to be flexible there.