Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-5432

Add SFM plugin for ngmix fitting

    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

            Hide
            jbosch Jim Bosch added a comment -

            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.

            Show
            jbosch Jim Bosch added a comment - 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.
            Hide
            nlust Nate Lust added a comment -

            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.

            Show
            nlust Nate Lust added a comment - 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.
            Hide
            pgee 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.

            Show
            pgee 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.
            Hide
            pgee Perry Gee added a comment -

            Review comments fixed, or reasons given

            Show
            pgee Perry Gee added a comment - Review comments fixed, or reasons given
            Hide
            swinbank John Swinbank added a comment -

            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.

            Show
            swinbank John Swinbank added a comment - 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.

              People

              • Assignee:
                pgee Perry Gee
                Reporter:
                jbosch Jim Bosch
                Reviewers:
                Nate Lust
                Watchers:
                Jim Bosch, John Swinbank, Nate Lust, Perry Gee
              • Votes:
                0 Vote for this issue
                Watchers:
                4 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:

                  Summary Panel