[PY-645][externa] Improved tolerance for dots in filenames & test linting #746
Conversation
| if len(suffixes) == 2: | ||
| image_id = str(filename).rstrip("".join(suffixes)) | ||
| elif len(suffixes) == 1: | ||
| image_id = str(filename.stem) |
There was a problem hiding this comment.
I'm a little unsure about this. Currently, if .nii.gz then we include this in the image_id
Otherwise, if .dcm or .nii, we don't include this in the image_id. Why is this?
There was a problem hiding this comment.
Currently, if .nii.gz then we include this in the image_id
Is that actually true? image_id = str(filename).rstrip("".join(suffixes)) would strip out .nii.gz 🤔
There was a problem hiding this comment.
Actually yes that is correct
Just tested and the behaviour now is:
- If any slot has a name in ending in anything other than
.nii,.dcm, or.nii.gz, the exporter fails (we want this) - The filename itself can be anything, and if it ends with any of the 3 above we strip it away
I believe this is correct
| if len(suffixes) == 2: | ||
| image_id = str(filename).rstrip("".join(suffixes)) | ||
| elif len(suffixes) == 1: | ||
| image_id = str(filename.stem) |
There was a problem hiding this comment.
Currently, if .nii.gz then we include this in the image_id
Is that actually true? image_id = str(filename).rstrip("".join(suffixes)) would strip out .nii.gz 🤔
darwin/exporter/formats/nifti.py
Outdated
| ) | ||
| else: | ||
| if not ( | ||
| filename.name.endswith(".nii.gz") |
There was a problem hiding this comment.
Can we also make sure we check that case-insensitive? That's gonna be next reported bug.
There was a problem hiding this comment.
Updated, all 6 checks are now case-insensitive
darwin/exporter/formats/nifti.py
Outdated
| if filename.name.lower().endswith(".nii.gz"): | ||
| image_id = str(filename).rstrip(".nii.gz") | ||
| elif filename.name.lower().endswith(".nii"): | ||
| image_id = str(filename).rstrip(".nii") |
There was a problem hiding this comment.
Will this work for case where we have upper cases? the condition will trigger, but the strip won't find the upper case extension, right?
There was a problem hiding this comment.
God I need to multitask less...
Updated. Now we check the lowered version of the filename so we're guaranteed to pick up any of the 3 matches
darwin/exporter/formats/nifti.py
Outdated
| elif len(suffixes) == 1: | ||
| image_id = str(filename.stem) | ||
| if filename.name.lower().endswith(".nii.gz"): | ||
| image_id = str(filename).lower().rstrip(".nii.gz") |
There was a problem hiding this comment.
Are you sure image_id can be a lower case of the filename? I'm not sure how it's used, I would personally prefer to just strip the extension in case-insensitive manner (I guess you may need regex for that with re.IGNORECASE). But if you're sure image_id can be modified this way, that's fine by me :)
There was a problem hiding this comment.
I don't believe it's an issue, but I cannot be not certain. Therefore, I've updated this so that we only strip the extension in a case-insensitive manner and leave the rest of the image_id untouched
Nathanjp91
changed the title
Problem
If the filename of either:
Solution
Introduce logic that explicitly looks for
.dcm,.nii, and.nii.gzextensions instead of pulling out suffixesChangelog