-
-
Notifications
You must be signed in to change notification settings - Fork 664
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
Add AnatomicalOrientation class #4804
base: master
Are you sure you want to change the base?
Add AnatomicalOrientation class #4804
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good so far!
This comment was marked as off-topic.
This comment was marked as off-topic.
2502620
to
fb83a56
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
Please move this conversation about MINC to #147 or another place on topic. |
fb83a56
to
4042eb3
Compare
4042eb3
to
3e2ef5b
Compare
@dzenanz Almost there. There is currently both the ToEnum, and FromEnum. The class "AnatomicalOrientation" can be seen to hold the representation of the orientation, and provide conversion with the Enum's, direction matrix, and perhaps the preferred handling it as 3 components. There is still the ToEnum string representation left over from the original DICOMOrientation class. That is |
I would like to have this. This convenience did not exist within ITK proper before, so there is no need for some sort of backwards compatibility. I wonder whether we should have a method to convert "To" string from a "From" string and vice versa. E.g. if an application has "RAI" string as a "From" code, it can call something like |
So your suggestion is to keep the ambiguous "ToEnum" character representation, and just add a method to convert To/From character representations? The concern here is really when |
If you want, you can remove the string constructor. Is there another similarly convenient way to initialize it? Something like: auto identityAO = AnatomicalOrientation::ToEnum("LPS");
AnatomicalOrientation ao(identityAO); |
More generally,: std::string toCode = "LPS";
auto aoEnum = AnatomicalOrientation::ToEnum(toCode);
AnatomicalOrientation ao(aoEnum); |
What is the status of this PR? I would like it to be finished/merged before it becomes stale, as PR revival usually involves extra effort. |
3e2ef5b
to
62a566b
Compare
@dzenanz Shall we have the SpatialOrientation filter changes be in another PR? |
I am fine with that. And that might even be better, to not hold up this PR. |
62a566b
to
7192104
Compare
When Legacy Remove is enabled deprecated warnings are generated causing one of the ITK Linux builds to failed. What is the expected behavior here? Addendum: Added ITK_LEGACY_SILENT support. |
4851faa
to
5060697
Compare
Current warning is due to testing of the deprecated methods:
Anyone know other locations in ITK which test deprecated methods with out warnings? |
This might be the answer you are looking for: ITK/Modules/Core/Common/test/itkSTLThreadTest.cxx Lines 18 to 20 in fb61d2c
|
Taking a look at the definition in |
5060697
to
de9110c
Compare
A uniform class for multiple descriptions of patient specifc orientations of 3D images.
Prefer describing orientation in IO classis with unambiguous CoordinateEnum triple.
de9110c
to
05127e3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is primarily rebase and squash? With removal of #ifdef ITK_AMBIGUOUS_TO_COORDINATE_ENUMS
?
Yes |
Mark as "ready for review"? |
The Documentation still needs work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice!!
Inline comment regarding method naming.
Wish list, could come later: *Long*
methods to work with the names as used in PrintSelf
.
} | ||
|
||
/** \brief Return the closest orientation for a direction cosine matrix. */ | ||
static ToEnum |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These following methods have asymmetry with regards to the currently names "ToEnum" and "FromEnum".
Perhaps the following would be consistent with the other "CreateFrom..Enum" methods:
static AnatomicalOrientation CreateFromDirectionCosines( const DirectionType &)
The class has an "operator<<" so it work sells as a ivar for ITK classes derived from LightObject with a PrintSelf methods. Could you please explain this further or be more specific. |
Move asymmetric conversion utility methods to protected functions. Add missing NegativeEnum methods for symmetry.
55d091f
to
b9b0a11
Compare
Yes, that is nice.
I was thinking: static const std::map<PositiveEnum, std::string> &
GetCodeToLongString();
// returns "left-to-right", for example
static const std::map<std::string, PositiveEnum> &
GetLongStringToCode()
// has "left-to-right", etc. keys |
This PR is a WIP, and more details will be added here when ready for discussion and review.
See #4788 for this issue and discussion of these changes.