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

WIP .hdr writer #31

Merged
merged 10 commits into from
Dec 5, 2018
Merged

WIP .hdr writer #31

merged 10 commits into from
Dec 5, 2018

Conversation

nilgoyette
Copy link
Collaborator

WIP because write_nifti is so ugly now that I didn't found the courage to make write_rgb_nifti work too. I would be really interested in your review. Like, how I can make this less ugly. Or even beautiful!

I added a lot of #[cfg(feature = "ndarray_volumes")] for is_hdr_file because it's currently only used with ndarray_volumes, but this will be removed "soon" when we support writing nifti-rs types.

Copy link
Owner

@Enet4 Enet4 left a comment

Choose a reason for hiding this comment

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

Well, in terms of what we currently have here, I don't happen to see anything egregiously bad or non-fancy. However, it would be nice to see how we can solve the major problems of writing files from here (as indicated in #19). Future versions should indeed become less and less independent from ndarray for writing.

src/writer.rs Outdated Show resolved Hide resolved
@nilgoyette
Copy link
Collaborator Author

What I don't like is the

if
    if
        write
    else
        write
else
    if
        write
    else
        write

and the fact that the same logic will also be duplicated in write_rgb_nifti. I thought that maybe there was a better solution but it doesn't seem to be the case :) I'll make it work for rgb and update this PR.

Copy link
Owner

@Enet4 Enet4 left a comment

Choose a reason for hiding this comment

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

Well, I can't think of a way to simplify those if-else statements without moving those pieces into separate functions, which I don't think is necessary. However, a few other things to consider below.

src/writer.rs Outdated Show resolved Hide resolved
src/writer.rs Outdated Show resolved Hide resolved
tests/writer.rs Show resolved Hide resolved
Copy link
Owner

@Enet4 Enet4 left a comment

Choose a reason for hiding this comment

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

Good to go. I have a few other concerns that I wish to attend before making a new release, but we'll deal with those separately. Thanks!

@Enet4 Enet4 merged commit 201a126 into Enet4:master Dec 5, 2018
@Enet4 Enet4 mentioned this pull request Dec 13, 2018
@nilgoyette nilgoyette deleted the hdr_writer branch June 4, 2020 13:26
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