# Setup Flux algorithms for testing with processCcd

XMLWordPrintable

#### Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s: None
• Labels:
• Story Points:
2
• Sprint:
Measurement Sprint 1
• Team:
Data Release Production

#### Description

Similar to DM-441 for flux algorithms GaussianFlux and NaiveFlux

Unit tests for major config options, just to be sure that they do something reasonable.

Test of at least one exception

#### Activity

Hide
Jim Bosch added a comment -

When you convert GaussianFlux, feel free to chop out the option that calls the SdssShape code internally if that's easier that converting it in its current form. In the polish stage, we'll want this to just rely on the shape flag, and if that's a lot easier than converting the more complex old implementation, we could consider jumping straight there.

Also, once we get ApertureFlux to the polished stage, I don't think there will be any need for NaiveFlux (or SincFlux, for that matter). I think we'll need them around for a while in the interim, because we won't be able to replace them with ApertureFlux until we can put multiple aperture fluxes in slots, and that may not happen until after we have aliases. But it may be a reason to give these less attention in even initial testing and cleanup than you otherwise might.

Show
Jim Bosch added a comment - When you convert GaussianFlux, feel free to chop out the option that calls the SdssShape code internally if that's easier that converting it in its current form. In the polish stage, we'll want this to just rely on the shape flag, and if that's a lot easier than converting the more complex old implementation, we could consider jumping straight there. Also, once we get ApertureFlux to the polished stage, I don't think there will be any need for NaiveFlux (or SincFlux, for that matter). I think we'll need them around for a while in the interim, because we won't be able to replace them with ApertureFlux until we can put multiple aperture fluxes in slots, and that may not happen until after we have aliases. But it may be a reason to give these less attention in even initial testing and cleanup than you otherwise might.
Hide
Perry Gee added a comment -

I am planning to do this ticket and review it simultaneously with DM-463. They can be reviewed by different people and checked in separately, but I plan to merge them to master at the same time.

Show
Perry Gee added a comment - I am planning to do this ticket and review it simultaneously with DM-463 . They can be reviewed by different people and checked in separately, but I plan to merge them to master at the same time.
Hide
Perry Gee added a comment -

I am putting this into In Review as well, as you might want to do this all at once. This is the additon of Gaussian Flux, which I need, plus several other algorithms which are less important. You can ask me to push those on to another issue if you prefer. They were sitting around and I wanted them to get in.

This is on top of tickets/DM-463, the other ticket I just put up for review.

[pgee@lsst-dev mb447]$git diff --stat tickets/DM-463 include/lsst/meas/base.h | 4 + include/lsst/meas/base/GaussianCentroid.h | 160 +++++++ include/lsst/meas/base/GaussianFlux.h | 172 +++++++ include/lsst/meas/base/NaiveCentroid.h | 190 ++++++++ include/lsst/meas/base/NaiveFlux.h | 160 +++++++ .../meas/base/algorithms/GaussianFluxTemplates.h | 103 ++++ .../lsst/meas/base/algorithms/NaiveFluxTemplates.h | 163 +++++++ python/lsst/meas/base/baseLib.i | 20 + python/lsst/meas/base/plugins.py | 4 + src/GaussianCentroid.cc | 116 +++++ src/GaussianFlux.cc | 165 +++++++ src/NaiveCentroid.cc | 138 ++++++ src/NaiveFlux.cc | 106 +++++ src/algorithms/all.h | 66 +++ src/algorithms/twodg.cc | 499 ++++++++++++++++++++ tests/testGaussianCentroidBasic.py | 129 +++++ tests/testGaussianFluxBasic.py | 104 ++++ tests/testNaiveCentroidBasic.py | 131 +++++ tests/testNaiveFluxBasic.py | 98 ++++ 19 files changed, 2528 insertions, 0 deletions Show Perry Gee added a comment - I am putting this into In Review as well, as you might want to do this all at once. This is the additon of Gaussian Flux, which I need, plus several other algorithms which are less important. You can ask me to push those on to another issue if you prefer. They were sitting around and I wanted them to get in. This is on top of tickets/ DM-463 , the other ticket I just put up for review. [pgee@lsst-dev mb447]$ git diff --stat tickets/ DM-463 include/lsst/meas/base.h | 4 + include/lsst/meas/base/GaussianCentroid.h | 160 +++++++ include/lsst/meas/base/GaussianFlux.h | 172 +++++++ include/lsst/meas/base/NaiveCentroid.h | 190 ++++++++ include/lsst/meas/base/NaiveFlux.h | 160 +++++++ .../meas/base/algorithms/GaussianFluxTemplates.h | 103 ++++ .../lsst/meas/base/algorithms/NaiveFluxTemplates.h | 163 +++++++ python/lsst/meas/base/baseLib.i | 20 + python/lsst/meas/base/plugins.py | 4 + src/GaussianCentroid.cc | 116 +++++ src/GaussianFlux.cc | 165 +++++++ src/NaiveCentroid.cc | 138 ++++++ src/NaiveFlux.cc | 106 +++++ src/algorithms/all.h | 66 +++ src/algorithms/twodg.cc | 499 ++++++++++++++++++++ tests/testGaussianCentroidBasic.py | 129 +++++ tests/testGaussianFluxBasic.py | 104 ++++ tests/testNaiveCentroidBasic.py | 131 +++++ tests/testNaiveFluxBasic.py | 98 ++++ 19 files changed, 2528 insertions , 0 deletions
Hide
Perry Gee added a comment -

Oops. Forgot to push my last set of comment cleanups

Show
Perry Gee added a comment - Oops. Forgot to push my last set of comment cleanups
Hide
Jim Bosch added a comment -
• On git commit messages: it's considered best practice to format git commit messages as a single summary line followed by two newlines and then a more complete description. It's okay to skip the more complete description for small changes, but the first line should always stand on its own. It's not worth fixing that in this case, probably (that'd require lots of rebasing), but it's a habit worth getting into in the future. As an example of why this is useful, try using "git log --oneline" on this branch, and note that one really can't get much from the last commit's message.
• src/all.h seems to have made a reappearance. If the code in it is needed, please move it to a .cc file (or, if necessary, move the .h to include/).
• GaussianFluxTemplates.h: useless comment by namespace close braces. But is this another case where we could just dump the contents of this file into GaussianFlux, and save ourselves some cleanup work later?
• NaiveFluxTemplates.h: never put anonymous namespaces in .h files; they only work if the header is only ever included by one .cc file, and in that case it's often best to just put the contents of the header into that .cc (which is, again, what I think we should do here).
• GaussianCentroid.h, NaiveCentroid.h: more of those unnecessary C standard library headers. Also see a lot of comments copied over from PsfFlux that talk about serving as an example, and others apparently copied over from ApertureFlux talking about measuring multiple fluxes. Better to have no docstrings than to have incorrect ones.
• All .cc files: more of those Doxygen \cond tricks that don't actually work. Please remove them.
• GaussianFlux.cc: I think our usual pattern for temporarily disabling code is to use #if 0 blocks, not comments.
• Test code: as in DM-463, lots of illegal variable names.
Show
Hide
Perry Gee added a comment -

I ignored the comments about restructuring the arrangement of code which came over from meas_algorithms. Except for removing the two remaining anonymous namespaces in include files and the one about all.h. That work is for a later ticket.

Did not fix the illegal variable names. The tests in general will be reworked on a later ticket. The ones being checked in are temporary.

Did not remove the comments around code that is disabled. It is not know to compile. It is there for later reference, and will be removed by the end of S14

Did everything else, I believe.

Show
Perry Gee added a comment - I ignored the comments about restructuring the arrangement of code which came over from meas_algorithms. Except for removing the two remaining anonymous namespaces in include files and the one about all.h. That work is for a later ticket. Did not fix the illegal variable names. The tests in general will be reworked on a later ticket. The ones being checked in are temporary. Did not remove the comments around code that is disabled. It is not know to compile. It is there for later reference, and will be removed by the end of S14 Did everything else, I believe.

#### People

Assignee:
Perry Gee
Reporter:
Perry Gee
Reviewers:
Jim Bosch
Watchers:
Jim Bosch, Perry Gee