-
Notifications
You must be signed in to change notification settings - Fork 262
tst: explicitly set intent codes to allow proper loading #604
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
b560da6
fix: explicitly set intent codes to allow proper loading
mgxd 8d5d70d
fix: do not allow saving of images without intent
mgxd fc619c0
fix: use unknown CIFTI type, raise ValueError if incorrect intent code
mgxd 4ab086b
enh: move cifti intent check into to_file_map
mgxd 0036e0c
enh: set default intent code when saving CIFTI images
mgxd e995d50
STY: remove unused import
mgxd 5f6b64f
sty: utilize assert_equals
mgxd ba81e30
fix: use correct CIFTI intent names
mgxd File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this a valid CIFTI type? I cannot find *plabel.nii in the specifications
https://www.nitrc.org/forum/attachment.php?attachid=341&group_id=454&forum_id=1955
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is a plabel: https://github.com/Washington-University/workbench/blob/master/src/Commands/CommandParser.cxx#L661
but indeed i don't see it in the spec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also here: https://github.com/Washington-University/workbench/blob/6c7414d9ba379604c10673d6d68eeeb68f1332e9/src/Common/DataFileTypeEnum.cxx#L207
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we will raise an exception upon saving, we have some options
ci.save
's signature to include a new parameter,force=False
, that can be switched on to allow saving without intent codesWDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about skipping the plabel test for now till we find the INTENT code. i would rather not have a parameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@effigies i had no luck finding it as well
https://github.com/Washington-University/workbench/blob/1b79e569e8b8277f4abf71fabd4afc2889849362/src/FilesBase/nifti2.h#L32-L49
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plabel is not in the cifti-2 spec. We didn't see it being useful at the time, so it was not in the draft. We later found that the information needed to reorder parcellated files to group certain parcels together in matrix display could be stored as a parcels by labels file, and decided to use that extention, matching the previous pattern of extension naming.
Per the "unknown" special cifti intent (main cifti-2 document, top of page 13, middle of paragraph), it is in fact valid to make such a file, and its extension is technically open to be anything that ends in ".<something>.nii". Feel free to write a test that loads and/or saves a cifti mapping combination that is not in the spec, this is an expected use case.