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

RGB images writer #26

Merged
merged 10 commits into from
Nov 22, 2018
Merged

RGB images writer #26

merged 10 commits into from
Nov 22, 2018

Conversation

nilgoyette
Copy link
Collaborator

There's some code copy, but I don't know how to avoid it. Most types can have a slope so the standard functions respect:

 where
    ...
    A: Div<Output = A>,
    A: FromPrimitive,
    A: ScalarOperand,
    A: Sub<Output = A>
    ...

Of course, [u8; 3] can't respect those traits, so I added write_rgb_nifti, which is almost identical to write_nifti, and I try to re-use utility functions as much as possible.

Moreover, I added 2 tests where I write RGB images, but I can't read them... So, those tests only check that the code compiles I guess. We will need a RGB reader someday to fix this!

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.

Sorry for the delay, here it is. I do feel that there should be a centralized way to write any kind of volume, but we can tackle that once we also have other missing pieces (namely, reading/writing more kinds of volumes).

src/writer.rs Outdated Show resolved Hide resolved
src/writer.rs Outdated Show resolved Hide resolved
src/writer.rs Outdated Show resolved Hide resolved
src/writer.rs Outdated Show resolved Hide resolved
src/writer.rs Outdated Show resolved Hide resolved
src/writer.rs Outdated Show resolved Hide resolved
let slope = A::from_f32(slope).unwrap();
let inter = A::from_f32(header.scl_inter).unwrap();
let slope = T::from_f32(slope).unwrap();
let inter = T::from_f32(header.scl_inter).unwrap();
Copy link
Owner

Choose a reason for hiding this comment

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

I think this part of the function should be reiterated (either now or in a separate PR), since it does not exactly resemble the current logic employed when reading transformed data. The data::element module provides multiple linear transformation strategies for each valid element data type to prevent precision loss. In this particular code, an scl_slope of 0.5 (haven't even seen real data with this, but it's a valid one nevertheless) would turn all voxel values to 0 if we pass an ndarray of integers. We might need a bit more code than just reusing this module though, since the transformation strategies currently live in immaterial types.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ouch. True. I'll check if I can copy your design.

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

TODO, in another PR, if you agree

  • Support hdr writing
  • Inverse Linear Transform

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.

Sounds good, thank you! 👍 It might be worth filing an issue for this concern, so we don't forget to fix it.

@Enet4 Enet4 merged commit 939fb82 into Enet4:master Nov 22, 2018
@Enet4 Enet4 mentioned this pull request Sep 5, 2019
@nilgoyette nilgoyette deleted the rgb_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