-
-
Notifications
You must be signed in to change notification settings - Fork 666
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
ENH: Set Seeds as conntainer objects #4793
base: master
Are you sure you want to change the base?
ENH: Set Seeds as conntainer objects #4793
Conversation
Best practices are to set member variables as object, and not repeat accessor methods in the interface of the owning object e.g. AddSeed, ClearSeed.
@jhlegarreta I was surprised to see These were the quick changes I made to have the seeds list behave like objects with setter/getters. Further investigation found other region growing filters with similar interfaces:
These classes seem to use std::vector to hold the seed lists. Perhaps performance was a reason for this approach? Modern C++ could do something like What can be done to improve the interface of these filters and make them more consistent? |
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.
Really sorry @blowekamp, looks like I was confused by the docstring in the implementation file, which documented the method as being deprecated:
d58bf56#diff-9daea1a5d3498baf38e16ef9fd06da89d2f898c4b599a7eb287a76fc3a720accL105
d58bf56#diff-9daea1a5d3498baf38e16ef9fd06da89d2f898c4b599a7eb287a76fc3a720accL137
But after investigation, I now see that the documentation was not updated and the passage left inadvertently in
c005b54
So please, go ahead and update the documentation.
If possible, can the methods be tested, please?
void | ||
AddSeed2(const IndexType & seed); | ||
|
||
/** Set a single seed point 2. | ||
* | ||
* This seed will be isolated from Seed1 (if possible). | ||
* | ||
* \deprecated Please use AddSeed2. | ||
* \deprecated Please use SetSeed2. |
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 should then say SetSeeds2
.
Best practices are to set member variables as object, and not repeat accessor methods in the interface of the owning object e.g. AddSeed, ClearSeed.
PR Checklist
Refer to the ITK Software Guide for
further development details if necessary.