Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add functionality to skip the four bytes flag and byteswap when read/write extensions #76

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

calvinchai
Copy link
Contributor

No description provided.

@Tokazama
Copy link
Member

I appreciate the PRs and would be happy to review them, but could you provide some more context?

For each PR we should have at least the reason why the change is needed. In cases like this, there are also some design decision to the problem I anticipate you are trying to solve.

@calvinchai
Copy link
Contributor Author

This change is necessary to ensure consistency between the behavior of the Julia implementation and Python's NiBabel library when handling NIfTI file extensions, particularly in how the extensions' binary data are written.

In Python's NiBabel library, the first four bytes (often referred to as the "extension flag") are handled differently. Specifically:

NiBabel does not write the first four bytes when serializing extensions into binary blobs. The first byte is trivially set to 1 if any extension is present, and the remaining three bytes are unused.

Additionally, NiBabel provides a byteswap option to control byte order. This is important for ensuring consistency, especially when the byte order of the extensions must match the header's byte order. By default, NiBabel writes data in the machine's native byte order, but in certain cases (such as ensuring consistency across a dataset), it’s necessary to use a specific byte order.

In our use case, we are building libraries for both Python and Julia, and we need the two implementations to behave as consistently as possible so the dataset can be used with either implementation.

@Tokazama
Copy link
Member

That all makes sense and I've wanted to address this for a while. So I really appreciate the contribution! I'm traveling right now and will do a more thorough review later, but for now I'd just suggest writing some tests so that we can ensure this new feature doesn't accidentally break in the future.

@calvinchai
Copy link
Contributor Author

Thanks for your response. I have updated the test cases, I think this one and #75 is good to go. #62 can be closed as well.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants