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

Setup Flux algorithms for testing with processCcd

    XMLWordPrintable

    Details

    • Type: Story
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: None
    • Labels:

      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

        Attachments

          Activity

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

          Oops. Forgot to push my last set of comment cleanups

          Show
          pgee Perry Gee added a comment - Oops. Forgot to push my last set of comment cleanups
          Hide
          jbosch 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
          jbosch 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.
          Hide
          pgee 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
          pgee 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:
            pgee Perry Gee
            Reporter:
            pgee Perry Gee
            Reviewers:
            Jim Bosch
            Watchers:
            Jim Bosch, Perry Gee
            Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

              Dates

              Created:
              Updated:
              Resolved:

                Jenkins

                No builds found.