Details
-
Type:
Story
-
Status: Done
-
Resolution: Done
-
Fix Version/s: None
-
Component/s: None
-
Labels:None
-
Story Points:10
-
Epic Link:
-
Sprint:DRP X16-3, DRP F16-1
-
Team:Data Release Production
Description
Add an SFM pluggin for ngmix fitting using one of the simple fitters in ngmix/fitting.py.
This should depend on DM-5429 (or a suitably configured modelfit_ShapeletPsfApprox) for approximating the PSF as a sum of Gaussians.
Testing and tuning this algorithm to get it working well should be deferred to another issue.
Attachments
Issue Links
Activity
I am happy to differ to Jim on this, though I do think that at some point we should probably start profiling our code to find out the true impacts and run times of things. I think that even a 5% difference over millions of sources can have a significant impact. And not that this is the case here, but it would be bad if say the python side of things ended up having the same approximate runtime as the c++ side of things. That would either mean more stuff should be in c++ or that the python layer needs to be smarter or more efficient.
If there is truly a performance issue, it would be a good idea to make programmers aware of it. As I say, there is tons of code in the stack written in the chained getter style. If this needs to be changed, profiling should be done sooner rather than later to make the case to everyone.
Also, I don't think that most programmers are aware of the difference in cost between:
array = exposure.getMaskedImage().getImage().getArray(); shape = array.shape (suppose that array is used several times)
and calling exposure.getMaskedImage().getImage().getArray().shape every time.
There is an ethos in Java (and other OO languages) that one should always call the getter, not use convenient instance variables.
People using compiled languages are used to the support of optimizers, and have developed "bad" habits. So if they have to be retrained, we have to make an issue of this.
Perry Gee
added a comment - If there is truly a performance issue, it would be a good idea to make programmers aware of it. As I say, there is tons of code in the stack written in the chained getter style. If this needs to be changed, profiling should be done sooner rather than later to make the case to everyone.
Also, I don't think that most programmers are aware of the difference in cost between:
array = exposure.getMaskedImage().getImage().getArray(); shape = array.shape (suppose that array is used several times)
and calling exposure.getMaskedImage().getImage().getArray().shape every time.
There is an ethos in Java (and other OO languages) that one should always call the getter, not use convenient instance variables.
People using compiled languages are used to the support of optimizers, and have developed "bad" habits. So if they have to be retrained, we have to make an issue of this. Review comments fixed, or reasons given
Perry Gee
added a comment - Review comments fixed, or reasons given Hooray – it's awesome this is marked done. Thanks Perry Gee!
Just a reminder of the guidance on commit messages though – please try to keep your summary lines to less than 50 characters.
I don't think we should start considering Python getters to be a performance issue, even if the involve going across the C++/Python boundary (or rather, if we find a getter that is a performance bottleneck, we need to fix it, because it probably meets we're copying something we shouldn't). On the other hand, I'm don't think that long chains of getters is are always easier to read than a bunch of temporary variables (it depends on how much you have of either). Ultimately, I think we should add properties and/or method forwarding to solve the readability issue, so something like exposure.image.array.shape is the most you'd need to write.