-
-
Notifications
You must be signed in to change notification settings - Fork 665
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
PERF: Remove unnecessary computations and parallelize function. #3681
PERF: Remove unnecessary computations and parallelize function. #3681
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.
LGTM, but someone else should review too.
Thanks @dzenanz . And, yes, definitely want to be cautious with merging this. |
I need more time to look into the code but I also see that "m_OutputPointData" has been removed and the Update method is modifying the input. Does this filter effectively only run "InPlace" now? That seems like a potentially unexpected change in behavior. Additionally, I'd expect a flag for that option similar to the InPlaceImageFilter. |
Hey @blowekamp , thanks for looking. Nice catch. Let me point out that this is a PointSetToImageFilter so the output is an image, not a point set. The contents of the point set, m_OutputPointData, were never returned to the user. Although I'm not sure, I believe this was due to a misunderstanding on my part when I originally wrote the code back in 2005 as someone who had been familiar with ITK less than a year. I lean towards removing it because there's no need for that variable and I think it hinders code readability. Also, actually computing its contents constitutes the unnecessary computations portion of the old code that this contribution is meant to address. |
Also, @blowekamp , your observation about overwriting the input data points to an important issue that should also be discussed. Both the current and proposed filter alter the input but only for the case of multiple fitting levels (so N4 and B-spline SyN image registration use cases do not apply). For multiple fitting levels, at the end of the filter processing, m_InputImageData will contain the residuals (i.e., difference between the input point set values and their B-spline approximation). Whether it was newbie ignorance, deviation from the original design intent of the filter, poor design choices, or some sort of combination, I certainly wouldn't design the filter this way today. Rather, I would copy the input point values to a different filter variable, e.g., m_ResidualPointSetValues, for use with multi-level fitting and leave the input alone. But that would change the behavior of the current filter, albeit in relatively less-frequent circumstances. As the filter is written now, it should be exactly equivalent, in terms of input and output, to the proposed changes. |
@ntustison I think that we could classify overwriting the input as a longstanding "bug" and fix that as a separate patch. That's just my opinion. |
Brad, could you merge this when happy with it? Otherwise provide more feedback. |
Thanks @hjmjohnson . Sounds good to me. |
H/t to @NitramNimod and @chrisadamsonmcri for pointing out the computational bottleneck that precipitated this contribution. Further discussion is here.
Two improvements:
For single-level approximation, no calls to UpdatePointSet are needed. This effects well-used code (e.g., N4 bias field correction) so I want to be cautious in submitting but the computational improvements should be relatively significant. For @thewtex , this would affect your B-spline image gradient module so please take a look if you have time.
PR Checklist