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

STYLE: Encapsulate NiftiImage qto_xyz mat44 data as Matrix<float,4,4> #3327

Conversation

N-Dekker
Copy link
Contributor

@N-Dekker N-Dekker commented Mar 23, 2022

Using itk::Matrix<float, 4, 4> (instead of std::vector<float>) to store the "qto_xyz" data from m_NiftiImage, as this Matrix template instantiation more closely resembles the original niftilib mat44 data structure.

Follow-up to pull request #3242 commit 9eb4c12 "ENH: bug #3241: Add qfac and qto_xyz to itkNiftiImageIO metadata" by Sean McBride (@seanm), which was merged on March 17, 2022.

@N-Dekker N-Dekker requested a review from seanm March 23, 2022 16:23
Using `itk::Matrix<float, 4, 4>` (instead of `std::vector<float>`) to
store the "qto_xyz" data from m_NiftiImage, as this `Matrix` template
instantiation more closely resembles the original niftilib `mat44` data
structure.
@N-Dekker N-Dekker force-pushed the Encapsulate-NiftiImage-qto_xyz-as-Matrix branch from 6eb549b to e960125 Compare March 23, 2022 17:08
@github-actions github-actions bot added area:Core Issues affecting the Core module area:IO Issues affecting the IO module type:Style Style changes: no logic impact (indentation, comments, naming) labels Mar 23, 2022
@N-Dekker N-Dekker marked this pull request as ready for review March 23, 2022 18:40
@seanm
Copy link
Contributor

seanm commented Mar 23, 2022

OK, give me a chance to try this in my app, want to make sure it actually works for me...

@N-Dekker
Copy link
Contributor Author

OK, give me a chance to try this in my app, want to make sure it actually works for me...

Thanks @seanm I'm not in a hurry 😃

If you want be sure than it does not get merged accidentally, you may do Review changes -> Request changes, and then switch to Review changes -> Approve once you're sure it works for you.

BTW, instead of using Matrix<float,4,4>, we might now also store the data of "qto_xyz" as a raw C-style array float[4][4], if you would like that better. But I think using Matrix<float,4,4> is most convenient in this case, as itk::Matrix<T,Rows,Columns> is a regular, copyable C++ class type, supporting the most common matrix operations, and interaction with vnl_matrix

@seanm
Copy link
Contributor

seanm commented Mar 24, 2022

OK, I've tested, this is good for me.

(Calling it "STYLE" seems a little light though, it is a behaviour and public API change, even though the old way didn't ship in a release.)

@hjmjohnson hjmjohnson merged commit e00bad0 into InsightSoftwareConsortium:master Mar 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Core Issues affecting the Core module area:IO Issues affecting the IO module type:Style Style changes: no logic impact (indentation, comments, naming)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants