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.

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

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

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.

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?

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?

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

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.

Sorry, missed that comment. thanks.

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.

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)

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

Two questions for Jim Bosch

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.

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.

Nate Lust

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.

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

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.

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.

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.

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.

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.