Skip to content

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants