Skip to content
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

BUG: Missing writing of the nifti descrip field. #4810

Merged
merged 1 commit into from
Aug 21, 2024

Conversation

zivy
Copy link
Member

@zivy zivy commented Aug 20, 2024

The nifti descrip field is currently only read and exposed via the ITK_FileNotes metadata dictionary variable. Setting the ITK_FileNotes via the metadata dictionary has no effect while writing. This commit writes the contents of the ITK_FileNotes variable if it is found in the metadata dictionary.

@github-actions github-actions bot added type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct area:IO Issues affecting the IO module labels Aug 20, 2024
@zivy zivy requested a review from lassoan August 20, 2024 14:36
Copy link
Member

@dzenanz dzenanz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@zivy
Copy link
Member Author

zivy commented Aug 21, 2024

Hi @dzenanz, please rerun the failed test if needed (it isn't related to this change) and merge when it passes. Thanks.

@dzenanz
Copy link
Member

dzenanz commented Aug 21, 2024

This will need to be rebased on top of #4811, after it gets merged. I plan to merge that as soon as its CI finished.

@blowekamp
Copy link
Member

@dzenanz I did a quick search and didn't find it. But for patches for 5.4 what is the order of merging for master, release, releaes-5.4?

@dzenanz
Copy link
Member

dzenanz commented Aug 21, 2024

I think merge into most specific first. So first merge topic branch into release-5.4, then merge release-5.4 into release, and finally merge release into master.

@dzenanz
Copy link
Member

dzenanz commented Aug 21, 2024

#4811 has now been merged. A plain rebase should be sufficient for a green CI.

The nifti descrip field is currently only read and exposed via the
ITK_FileNotes metadata dictionary variable. Setting the
ITK_FileNotes via the metadata dictionary has no effect while writing.
This commit writes the contents of the ITK_FileNotes variable if it is
found in the metadata dictionary.
@zivy
Copy link
Member Author

zivy commented Aug 21, 2024

Rebased and force pushed.

@zivy zivy merged commit 9b7d005 into InsightSoftwareConsortium:release-5.4 Aug 21, 2024
13 checks passed
@dzenanz
Copy link
Member

dzenanz commented Aug 21, 2024

I merged it to release and master branches.

@zivy
Copy link
Member Author

zivy commented Aug 21, 2024

Thanks. Much appreciated.

@zivy zivy deleted the bugNiftiIO branch August 21, 2024 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:IO Issues affecting the IO module type:Bug Inconsistencies or issues which will cause an incorrect result under some or all circumstances type:Testing Ensure that the purpose of a class is met/the results on a wide set of test cases are correct
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants