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

Add SFM plugin for ngmix fitting

    XMLWordPrintable

Details

    • Story
    • Status: Done
    • Resolution: Done
    • None
    • None
    • None
    • 10
    • DRP X16-3, DRP F16-1
    • 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

            Should be configurable to use either exponential or deV.

            swinbank John Swinbank added a comment - Should be configurable to use either exponential or deV.

            We discussed this at the 2016-05-26 planning meeting, where Perry reported that he has already done some work on it.

            swinbank John Swinbank added a comment - We discussed this at the 2016-05-26 planning meeting, where Perry reported that he has already done some work on it.
            pgee Perry Gee added a comment -

            This plugin is in tickets/DM-5432. It depends on tickets/DM-6123, which I also gave you.

            pgee Perry Gee added a comment - This plugin is in tickets/ DM-5432 . It depends on tickets/ DM-6123 , which I also gave you.
            pgee Perry Gee added a comment -

            nlust Are you able to review this one. It is really John who has a need for it to be done soon. If you can't I will try to find someone else.

            pgee Perry Gee added a comment - nlust Are you able to review this one. It is really John who has a need for it to be done soon. If you can't I will try to find someone else.

            I don't yet know if & when Nate will get around to this – if it's not within a day or two, please let me know and I'll help you find somebody else.

            However, it does look like there are quite a few minor formatting errors in the code. These will need to be fixed before you can merge to master, and fixing them before review will make the code easier for the reviewer to read – so, if you have time, why not do it while you're waiting?

            $ pyflakes-2.7 LMSimpleShape.py
            LMSimpleShape.py:27: 'numpy' imported but unused
            LMSimpleShape.py:29: 'ngmix.bootstrap.EMRunner' imported but unused
            LMSimpleShape.py:37: 'lsst.afw.geom as afwGeom' imported but unused
            LMSimpleShape.py:38: 'lsst.afw.math as afwMath' imported but unused
            LMSimpleShape.py:39: 'lsst.afw.image as afwImage' imported but unused
            LMSimpleShape.py:45: 'lsst.meas.base.FlagDefinition' imported but unused
            LMSimpleShape.py:45: 'lsst.meas.base.FlagHandler' imported but unused
            LMSimpleShape.py:45: 'lsst.meas.base.FlagDefinitionVector' imported but unused
            

            $ pep8-2.7 --max-line-length=110 --ignore=E133,E226,E228,E251,N802,N803,W391 LMSimpleShape.py
            LMSimpleShape.py:30:35: E231 missing whitespace after ','
            LMSimpleShape.py:30:54: E231 missing whitespace after ','
            LMSimpleShape.py:30:69: E231 missing whitespace after ','
            LMSimpleShape.py:31:18: E231 missing whitespace after ','
            LMSimpleShape.py:31:36: E231 missing whitespace after ','
            LMSimpleShape.py:52:1: E302 expected 2 blank lines, found 1
            LMSimpleShape.py:55:17: E128 continuation line under-indented for visual indent
            LMSimpleShape.py:60:35: E128 continuation line under-indented for visual indent
            LMSimpleShape.py:62:35: E127 continuation line over-indented for visual indent
            LMSimpleShape.py:64:35: E127 continuation line over-indented for visual indent
            LMSimpleShape.py:66:1: E302 expected 2 blank lines, found 1
            LMSimpleShape.py:68:9: E128 continuation line under-indented for visual indent
            LMSimpleShape.py:76:5: E123 closing bracket does not match indentation of opening bracket's line
            LMSimpleShape.py:92:5: E301 expected 1 blank line, found 0
            LMSimpleShape.py:96:21: E124 closing bracket does not match visual indentation
            LMSimpleShape.py:97:12: E713 test for membership should be 'not in'
            LMSimpleShape.py:110:19: E128 continuation line under-indented for visual indent
            LMSimpleShape.py:115:12: E714 test for object identity should be 'is not'
            LMSimpleShape.py:117:62: E128 continuation line under-indented for visual indent
            LMSimpleShape.py:130:18: E225 missing whitespace around operator
            LMSimpleShape.py:131:16: E225 missing whitespace around operator
            LMSimpleShape.py:134:20: E225 missing whitespace around operator
            LMSimpleShape.py:134:37: E231 missing whitespace after ','
            LMSimpleShape.py:136:21: E225 missing whitespace around operator
            LMSimpleShape.py:138:25: E225 missing whitespace around operator
            LMSimpleShape.py:147:20: E225 missing whitespace around operator
            LMSimpleShape.py:167:18: E225 missing whitespace around operator
            LMSimpleShape.py:187:72: E231 missing whitespace after ','
            LMSimpleShape.py:190:15: E225 missing whitespace around operator
            LMSimpleShape.py:190:25: E231 missing whitespace after ':'
            LMSimpleShape.py:190:52: E231 missing whitespace after ':'
            LMSimpleShape.py:190:77: E231 missing whitespace after ':'
            LMSimpleShape.py:191:15: E225 missing whitespace around operator
            LMSimpleShape.py:193:12: E225 missing whitespace around operator
            LMSimpleShape.py:235:83: E231 missing whitespace after ','
            

            $ pyflakes-2.7 testLMSimpleShape.py
            testLMSimpleShape.py:23: 're' imported but unused
            testLMSimpleShape.py:23: 'sys' imported but unused
            testLMSimpleShape.py:24: 'glob' imported but unused
            testLMSimpleShape.py:25: 'math' imported but unused
            testLMSimpleShape.py:28: 'itertools' imported but unused
            testLMSimpleShape.py:29: 'lsst.pex.exceptions as pexExceptions' imported but unused
            testLMSimpleShape.py:31: 'lsst.afw.math as afwMath' imported but unused
            testLMSimpleShape.py:33: 'lsst.afw.detection as afwDetection' imported but unused
            testLMSimpleShape.py:35: 'lsst.afw.geom as afwGeom' imported but unused
            testLMSimpleShape.py:36: 'lsst.afw.geom.ellipses as afwEll' imported but unused
            testLMSimpleShape.py:37: 'lsst.afw.coord as afwCoord' imported but unused
            testLMSimpleShape.py:38: 'lsst.afw.display.ds9' imported but unused
            testLMSimpleShape.py:41: 'lsst.meas.algorithms' imported but unused
            testLMSimpleShape.py:148: 'lsst.meas.modelfit' imported but unused
            

            $ pep8-2.7 --max-line-length=110 --ignore=E133,E226,E228,E251,N802,N803,W391 testLMSimpleShape.py
            testLMSimpleShape.py:23:10: E401 multiple imports on one line
            testLMSimpleShape.py:45:1: E265 block comment should start with '# '
            testLMSimpleShape.py:47:1: E302 expected 2 blank lines, found 1
            testLMSimpleShape.py:56:87: E231 missing whitespace after ','
            testLMSimpleShape.py:68:12: E714 test for object identity should be 'is not'
            testLMSimpleShape.py:70:12: E714 test for object identity should be 'is not'
            testLMSimpleShape.py:72:12: E714 test for object identity should be 'is not'
            testLMSimpleShape.py:75:12: E714 test for object identity should be 'is not'
            testLMSimpleShape.py:77:12: E714 test for object identity should be 'is not'
            testLMSimpleShape.py:80:12: E714 test for object identity should be 'is not'
            testLMSimpleShape.py:93:12: E714 test for object identity should be 'is not'
            testLMSimpleShape.py:139:46: E225 missing whitespace around operator
            testLMSimpleShape.py:195:1: E265 block comment should start with '# '
            testLMSimpleShape.py:197:1: E302 expected 2 blank lines, found 1
            testLMSimpleShape.py:207:1: E302 expected 2 blank lines, found 1
            

            (These errors were found by automatic tools; I've not checked them all by eye. That means that in some cases, what you've done may be ok – but it does need checking.)

            Also, note that you're using the old-style way of setting up unit tests. Refer to https://developer.lsst.io/coding/python_testing.html for the latest best practice. I don't think what you're doing is strictly incorrect, but I'd expect a reviewer to ask you to justify and/or change it, so maybe that's something you can do in advance.

            swinbank John Swinbank added a comment - I don't yet know if & when Nate will get around to this – if it's not within a day or two, please let me know and I'll help you find somebody else. However, it does look like there are quite a few minor formatting errors in the code. These will need to be fixed before you can merge to master, and fixing them before review will make the code easier for the reviewer to read – so, if you have time, why not do it while you're waiting? $ pyflakes-2.7 LMSimpleShape.py LMSimpleShape.py:27: 'numpy' imported but unused LMSimpleShape.py:29: 'ngmix.bootstrap.EMRunner' imported but unused LMSimpleShape.py:37: 'lsst.afw.geom as afwGeom' imported but unused LMSimpleShape.py:38: 'lsst.afw.math as afwMath' imported but unused LMSimpleShape.py:39: 'lsst.afw.image as afwImage' imported but unused LMSimpleShape.py:45: 'lsst.meas.base.FlagDefinition' imported but unused LMSimpleShape.py:45: 'lsst.meas.base.FlagHandler' imported but unused LMSimpleShape.py:45: 'lsst.meas.base.FlagDefinitionVector' imported but unused $ pep8-2.7 --max-line-length=110 --ignore=E133,E226,E228,E251,N802,N803,W391 LMSimpleShape.py LMSimpleShape.py:30:35: E231 missing whitespace after ',' LMSimpleShape.py:30:54: E231 missing whitespace after ',' LMSimpleShape.py:30:69: E231 missing whitespace after ',' LMSimpleShape.py:31:18: E231 missing whitespace after ',' LMSimpleShape.py:31:36: E231 missing whitespace after ',' LMSimpleShape.py:52:1: E302 expected 2 blank lines, found 1 LMSimpleShape.py:55:17: E128 continuation line under-indented for visual indent LMSimpleShape.py:60:35: E128 continuation line under-indented for visual indent LMSimpleShape.py:62:35: E127 continuation line over-indented for visual indent LMSimpleShape.py:64:35: E127 continuation line over-indented for visual indent LMSimpleShape.py:66:1: E302 expected 2 blank lines, found 1 LMSimpleShape.py:68:9: E128 continuation line under-indented for visual indent LMSimpleShape.py:76:5: E123 closing bracket does not match indentation of opening bracket's line LMSimpleShape.py:92:5: E301 expected 1 blank line, found 0 LMSimpleShape.py:96:21: E124 closing bracket does not match visual indentation LMSimpleShape.py:97:12: E713 test for membership should be 'not in' LMSimpleShape.py:110:19: E128 continuation line under-indented for visual indent LMSimpleShape.py:115:12: E714 test for object identity should be 'is not' LMSimpleShape.py:117:62: E128 continuation line under-indented for visual indent LMSimpleShape.py:130:18: E225 missing whitespace around operator LMSimpleShape.py:131:16: E225 missing whitespace around operator LMSimpleShape.py:134:20: E225 missing whitespace around operator LMSimpleShape.py:134:37: E231 missing whitespace after ',' LMSimpleShape.py:136:21: E225 missing whitespace around operator LMSimpleShape.py:138:25: E225 missing whitespace around operator LMSimpleShape.py:147:20: E225 missing whitespace around operator LMSimpleShape.py:167:18: E225 missing whitespace around operator LMSimpleShape.py:187:72: E231 missing whitespace after ',' LMSimpleShape.py:190:15: E225 missing whitespace around operator LMSimpleShape.py:190:25: E231 missing whitespace after ':' LMSimpleShape.py:190:52: E231 missing whitespace after ':' LMSimpleShape.py:190:77: E231 missing whitespace after ':' LMSimpleShape.py:191:15: E225 missing whitespace around operator LMSimpleShape.py:193:12: E225 missing whitespace around operator LMSimpleShape.py:235:83: E231 missing whitespace after ',' $ pyflakes-2.7 testLMSimpleShape.py testLMSimpleShape.py:23: 're' imported but unused testLMSimpleShape.py:23: 'sys' imported but unused testLMSimpleShape.py:24: 'glob' imported but unused testLMSimpleShape.py:25: 'math' imported but unused testLMSimpleShape.py:28: 'itertools' imported but unused testLMSimpleShape.py:29: 'lsst.pex.exceptions as pexExceptions' imported but unused testLMSimpleShape.py:31: 'lsst.afw.math as afwMath' imported but unused testLMSimpleShape.py:33: 'lsst.afw.detection as afwDetection' imported but unused testLMSimpleShape.py:35: 'lsst.afw.geom as afwGeom' imported but unused testLMSimpleShape.py:36: 'lsst.afw.geom.ellipses as afwEll' imported but unused testLMSimpleShape.py:37: 'lsst.afw.coord as afwCoord' imported but unused testLMSimpleShape.py:38: 'lsst.afw.display.ds9' imported but unused testLMSimpleShape.py:41: 'lsst.meas.algorithms' imported but unused testLMSimpleShape.py:148: 'lsst.meas.modelfit' imported but unused $ pep8-2.7 --max-line-length=110 --ignore=E133,E226,E228,E251,N802,N803,W391 testLMSimpleShape.py testLMSimpleShape.py:23:10: E401 multiple imports on one line testLMSimpleShape.py:45:1: E265 block comment should start with '# ' testLMSimpleShape.py:47:1: E302 expected 2 blank lines, found 1 testLMSimpleShape.py:56:87: E231 missing whitespace after ',' testLMSimpleShape.py:68:12: E714 test for object identity should be 'is not' testLMSimpleShape.py:70:12: E714 test for object identity should be 'is not' testLMSimpleShape.py:72:12: E714 test for object identity should be 'is not' testLMSimpleShape.py:75:12: E714 test for object identity should be 'is not' testLMSimpleShape.py:77:12: E714 test for object identity should be 'is not' testLMSimpleShape.py:80:12: E714 test for object identity should be 'is not' testLMSimpleShape.py:93:12: E714 test for object identity should be 'is not' testLMSimpleShape.py:139:46: E225 missing whitespace around operator testLMSimpleShape.py:195:1: E265 block comment should start with '# ' testLMSimpleShape.py:197:1: E302 expected 2 blank lines, found 1 testLMSimpleShape.py:207:1: E302 expected 2 blank lines, found 1 (These errors were found by automatic tools; I've not checked them all by eye. That means that in some cases, what you've done may be ok – but it does need checking.) Also, note that you're using the old-style way of setting up unit tests. Refer to https://developer.lsst.io/coding/python_testing.html for the latest best practice. I don't think what you're doing is strictly incorrect, but I'd expect a reviewer to ask you to justify and/or change it, so maybe that's something you can do in advance.
            nlust Nate Lust added a comment -

            Given the changes above that need to be made I will hold off reviewing this ticket until they are done. I will however not delay reviewing this and will get to it as soon as the changes are made, as to not delay completing this work.

            nlust Nate Lust added a comment - Given the changes above that need to be made I will hold off reviewing this ticket until they are done. I will however not delay reviewing this and will get to it as soon as the changes are made, as to not delay completing this work.
            pgee Perry Gee added a comment -

            I was out of the office yesterday. I will finish this up this morning.

            Are the automatic tools available somewhere so that I can save us all time by running them myself? This was discussed at Bremerton, but I never heard that they were done or available.

            Many of these syntax requests conflict with previous reviewer requests. Should I take them as authoritative?

            pgee Perry Gee added a comment - I was out of the office yesterday. I will finish this up this morning. Are the automatic tools available somewhere so that I can save us all time by running them myself? This was discussed at Bremerton, but I never heard that they were done or available. Many of these syntax requests conflict with previous reviewer requests. Should I take them as authoritative?
            swinbank John Swinbank added a comment - - edited

            Are the automatic tools available somewhere so that I can save us all time by running them myself?

            The two checkers I used were pep8 and pyflakes. You can install them on your system by following those links, and then run them directly. Alternatively, some people prefer to use an editor or IDE that does so automatically on their behalf – do whatever you're most comfortable with.

            Note that RFC-162 suggests some modifications to the pep8 defaults which make it better comply with LSST conventions (that's the --max-line-length=110 --ignore=E133,E226,E228,E251,N802,N803,W391 in the command lines I pasted above). However, in all cases, our documented coding standards take precedence over the output of tools: the latter provide guidance only.

            Many of these syntax requests conflict with previous reviewer requests. Should I take them as authoritative?

            As above, the tools provide guidance only: documented standards are authoritative. I would be surprised if reviewer requests contradict them in anything other than the most extraordinary circumstances, though. Can you provide some examples?

            swinbank John Swinbank added a comment - - edited Are the automatic tools available somewhere so that I can save us all time by running them myself? The two checkers I used were pep8 and pyflakes . You can install them on your system by following those links, and then run them directly. Alternatively, some people prefer to use an editor or IDE that does so automatically on their behalf – do whatever you're most comfortable with. Note that RFC-162 suggests some modifications to the pep8 defaults which make it better comply with LSST conventions (that's the --max-line-length=110 --ignore=E133,E226,E228,E251,N802,N803,W391 in the command lines I pasted above). However, in all cases, our documented coding standards take precedence over the output of tools: the latter provide guidance only. Many of these syntax requests conflict with previous reviewer requests. Should I take them as authoritative? As above, the tools provide guidance only: documented standards are authoritative. I would be surprised if reviewer requests contradict them in anything other than the most extraordinary circumstances, though. Can you provide some examples?
            pgee Perry Gee added a comment -

            OK, I made all the changes in John's list. Except for the lsst.meas.modelfit import in testLMSimple.py, which actually needs to be there. Sorry about all the junk in this checkin. A lot of it was not carefully cutting and pasting from Erin's code. I'm sure there are still other style issues because of that. Will look at them later today. Running these tools would be really useful.

            I would actually like the test against CModel to be run if meas_modelfit is imported, but not otherwise. I don't want to create a formal dependency on meas_modelfit.

            I'm not sure how to do this. Can I name meas_modelfit as optional in the cfg file for this package, then run the test conditionally?

            pgee Perry Gee added a comment - OK, I made all the changes in John's list. Except for the lsst.meas.modelfit import in testLMSimple.py, which actually needs to be there. Sorry about all the junk in this checkin. A lot of it was not carefully cutting and pasting from Erin's code. I'm sure there are still other style issues because of that. Will look at them later today. Running these tools would be really useful. I would actually like the test against CModel to be run if meas_modelfit is imported, but not otherwise. I don't want to create a formal dependency on meas_modelfit. I'm not sure how to do this. Can I name meas_modelfit as optional in the cfg file for this package, then run the test conditionally?

            OK, I made all the changes in John's list.

            Thank you!

            I'm not sure how to do this

            If you name the package as setupOptional in the EUPS table file (NB not .cfg), it will effectively be treated as a hard dependency by the eups distrib system – i.e., anybody installing meas_extensions_ngmix will get _modelfit as well. It's your call as to how much of a bad thing that would be.

            Otherwise, you can simply use the skipUnless decorator to probe for the package at run time. Something like the following should work:

            try:
                import lsst.meas.modelfit as modelfit
            except ImportError:
                modelfit = None
             
            [...]
                @unittest.skipUnless(modelfit)
                def testLMSimpleShapeSingleGaussian(self):
                    # Test code here
            

            (warning: untested. You'll probably need to tweak it.)

            swinbank John Swinbank added a comment - OK, I made all the changes in John's list. Thank you! I'm not sure how to do this If you name the package as setupOptional in the EUPS table file (NB not .cfg ), it will effectively be treated as a hard dependency by the eups distrib system – i.e., anybody installing meas_extensions_ngmix will get _modelfit as well. It's your call as to how much of a bad thing that would be. Otherwise, you can simply use the skipUnless decorator to probe for the package at run time. Something like the following should work: try: import lsst.meas.modelfit as modelfit except ImportError: modelfit = None   [...] @unittest.skipUnless(modelfit) def testLMSimpleShapeSingleGaussian(self): # Test code here (warning: untested. You'll probably need to tweak it.)
            pgee Perry Gee added a comment -

            Thanks, John. That's great.

            Don't forget to point me at your code scanning tools.

            pgee Perry Gee added a comment - Thanks, John. That's great. Don't forget to point me at your code scanning tools.

            Don't forget to point me at your code scanning tools.

            Sorry, perhaps I wasn't clear earlier – I don't have any code scanning tools other than pep8 and pyflakes, which I pointed you at above.

            swinbank John Swinbank added a comment - Don't forget to point me at your code scanning tools. Sorry, perhaps I wasn't clear earlier – I don't have any code scanning tools other than pep8 and pyflakes, which I pointed you at above.
            pgee Perry Gee added a comment -

            Sorry, missed that comment. thanks.

            pgee Perry Gee added a comment - Sorry, missed that comment. thanks.
            pgee Perry Gee added a comment -

            I made changes to DM-5432 to make the meas_modelfit tests optional, depending on meas_modelfit import

            Could not fix one of the pep8 complaints, but I think it is OK.

            Ready for review.

            pgee Perry Gee added a comment - I made changes to DM-5432 to make the meas_modelfit tests optional, depending on meas_modelfit import Could not fix one of the pep8 complaints, but I think it is OK. Ready for review.
            nlust Nate Lust added a comment -

            A lot of smallish comments to address but over all not too far off from being able to merge. Outside of the unit tests, have you tested this on real data and compared it to our other methods (as in this exact code, not just testing ngmix)

            nlust Nate Lust added a comment - A lot of smallish comments to address but over all not too far off from being able to merge. Outside of the unit tests, have you tested this on real data and compared it to our other methods (as in this exact code, not just testing ngmix)
            nlust Nate Lust added a comment -

            Shoot me a message before you do the merge, or if you have any questions on my comments.

            nlust Nate Lust added a comment - Shoot me a message before you do the merge, or if you have any questions on my comments.
            pgee Perry Gee added a comment -

            Two questions for jbosch

            What is the correct execution order static for a PsfApprox algorithm, and for a shape measurement algorithm. These are hardcoded to 1.0 and 3.0 in meas_modelfit (psf.py and cmodel.py). Are there standard BasePlugin numbers which are appropriate? SHAPE_ORDER doesn't even seem to be appropriate for the PsfApprox.

            At one time you told me to use RuntimeError for errors in plugins which were unexpected (in the case of Ngmix, no assigned value in the error enumeration Erin keeps). Nate asks if it is appropriate behavior, for such errors to throw out of the baseMeasurement loop.

            pgee Perry Gee added a comment - Two questions for jbosch What is the correct execution order static for a PsfApprox algorithm, and for a shape measurement algorithm. These are hardcoded to 1.0 and 3.0 in meas_modelfit (psf.py and cmodel.py). Are there standard BasePlugin numbers which are appropriate? SHAPE_ORDER doesn't even seem to be appropriate for the PsfApprox. At one time you told me to use RuntimeError for errors in plugins which were unexpected (in the case of Ngmix, no assigned value in the error enumeration Erin keeps). Nate asks if it is appropriate behavior, for such errors to throw out of the baseMeasurement loop.
            jbosch Jim Bosch added a comment -

            There is no standard execution order appropriate for CModel, but it has to run after SHAPE_ORDER. I'd recommend (SHAPE_ORDER+0.5), maybe?. PsfApprox can run at any point after CENTROID_ORDER and before CModel, so either SHAPE_ORDER or FLUX_ORDER would be fine.

            I think you could make a case for either LogicError or RuntimeError here, because this does indicate something you expect will never happen. The important thing is that neither of these is fatal (so a problem in ngmix can't bring down the whole pipeline), but they'll both generate a warning in the logs telling the user to file a bug report.

            jbosch Jim Bosch added a comment - There is no standard execution order appropriate for CModel, but it has to run after SHAPE_ORDER. I'd recommend (SHAPE_ORDER+0.5), maybe?. PsfApprox can run at any point after CENTROID_ORDER and before CModel, so either SHAPE_ORDER or FLUX_ORDER would be fine. I think you could make a case for either LogicError or RuntimeError here, because this does indicate something you expect will never happen. The important thing is that neither of these is fatal (so a problem in ngmix can't bring down the whole pipeline), but they'll both generate a warning in the logs telling the user to file a bug report.
            pgee Perry Gee added a comment -

            nlust

            I have checked in a new, improved version of the plugins to tickets/DM-5432. I either responded on the pull request, or did what was asked (or sometimes both)

            I will wait until morning to merge.

            pgee Perry Gee added a comment - nlust I have checked in a new, improved version of the plugins to tickets/ DM-5432 . I either responded on the pull request, or did what was asked (or sometimes both) I will wait until morning to merge.
            nlust Nate Lust added a comment - - edited

            the __all__ should be on one line, I found one more place with integer division, and made a comment on the pointers in variables. The first two should be a quick fix, and the later I think as this is a measurement plugin every bit of efficiency is a win, but if you think readability trumps that, I will not hold back the merge. Other than that I think it is good to go, thanks for making those changes

            nlust Nate Lust added a comment - - edited the __all__ should be on one line, I found one more place with integer division, and made a comment on the pointers in variables. The first two should be a quick fix, and the later I think as this is a measurement plugin every bit of efficiency is a win, but if you think readability trumps that, I will not hold back the merge. Other than that I think it is good to go, thanks for making those changes
            pgee Perry Gee added a comment -

            I will have a look at the division case. I was actually intending integer division in all of the cases that you cited originally (whole pixel coordinated), but I checked with Erin and was told that rounding the pixel coordinates down might slightly slow down the algorithm.

            I actually am more in agreement with you about the large layer of pointer calls and dereferences than you might think. I used to always write my code in the way that you suggest, and I'm willing to fix this code. However, the stack of code is full of stuff which looks like exposure.getMaskedImage().getImage().getArray(), so I have taken it to be the way things are done. This exact piece of code came up in Bremerton, if you recall.

            The alternative is code full of temporary variables named maskedImage, image, and array. If you are working with several exposures, or a Psf and an exposure at the same time, these tend to confuse the code.

            But rather than this single out this one case, could you give me an idea of what the cost for this kind of chained pointer call dereferencing to the other things that we have to do, such a call the measurement algorithms and fit the galaxies? If the cost is actually significant, I am more than happy to line up on the side of keeping temporary pointer variables around. And I really don't know.

            pgee Perry Gee added a comment - I will have a look at the division case. I was actually intending integer division in all of the cases that you cited originally (whole pixel coordinated), but I checked with Erin and was told that rounding the pixel coordinates down might slightly slow down the algorithm. I actually am more in agreement with you about the large layer of pointer calls and dereferences than you might think. I used to always write my code in the way that you suggest, and I'm willing to fix this code. However, the stack of code is full of stuff which looks like exposure.getMaskedImage().getImage().getArray(), so I have taken it to be the way things are done. This exact piece of code came up in Bremerton, if you recall. The alternative is code full of temporary variables named maskedImage, image, and array. If you are working with several exposures, or a Psf and an exposure at the same time, these tend to confuse the code. But rather than this single out this one case, could you give me an idea of what the cost for this kind of chained pointer call dereferencing to the other things that we have to do, such a call the measurement algorithms and fit the galaxies? If the cost is actually significant, I am more than happy to line up on the side of keeping temporary pointer variables around. And I really don't know.
            pgee Perry Gee added a comment -

            BTW, insofar as I have a system for handling these kinds of things, it is that I try to define a temporary variable if I am going to use a pointer more than once, and if the 2nd or 3rd use are nearby (in the same block). But I have not been saving pointers when the 2nd instance is far downstream, in a different scope or where the temporary pointer variable might cause confusion (be out of scope or where something might have changed the original object in the meantime). So your comment about fetching exposure.getMaskedImage().getImage().getArray().shape was entirely correct, that I could have saved the pointer in this case.

            pgee Perry Gee added a comment - BTW, insofar as I have a system for handling these kinds of things, it is that I try to define a temporary variable if I am going to use a pointer more than once, and if the 2nd or 3rd use are nearby (in the same block). But I have not been saving pointers when the 2nd instance is far downstream, in a different scope or where the temporary pointer variable might cause confusion (be out of scope or where something might have changed the original object in the meantime). So your comment about fetching exposure.getMaskedImage().getImage().getArray().shape was entirely correct, that I could have saved the pointer in this case.
            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.

            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.
            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.

            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.
            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.

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

            Review comments fixed, or reasons given

            pgee Perry Gee added a comment - Review comments fixed, or reasons given

            Hooray – it's awesome this is marked done. Thanks pgee!

            Just a reminder of the guidance on commit messages though – please try to keep your summary lines to less than 50 characters.

            swinbank John Swinbank added a comment - Hooray – it's awesome this is marked done. Thanks pgee ! Just a reminder of the guidance on commit messages though – please try to keep your summary lines to less than 50 characters.

            People

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

              Dates

                Created:
                Updated:
                Resolved:

                Jenkins

                  No builds found.