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

Aperture Flux back into an abstract class

    XMLWordPrintable

    Details

      Description

      The last change to ApertureFlux to make it work with the new C++ design changed ApertureFluxAlgorithm into an instantiatable class. However, I have now figured out how to make this work with SWIG while still allowing measure and fail to be defined by default at the ApertureFlux level.. So this issue is to put things back in order.

        Attachments

          Activity

          Ascending order - Click to sort in descending order
          Hide
          Permalink
          pgee Perry Gee added a comment -

          I think the version under u/pgee/DM-1659 is now correct. The error I made was to try to define fail() withing the base class ApertureFlux and measure() within the derived class CircularApertureFlux. That resulted in an undefined dynamic call from SWIG, for reasons I don't understand. The checked in version has both methods defined in the derived class.

          include/lsst/meas/base/ApertureFlux.h | 6 +++---
          include/lsst/meas/base/CircularApertureFlux.h | 8 +++++++-
          python/lsst/meas/base/ApertureFlux.i | 2 +-
          src/ApertureFlux.cc | 10 ----------
          src/CircularApertureFlux.cc | 4 ++++
          5 files changed, 15 insertions, 15 deletions

          Show
          pgee Perry Gee added a comment - I think the version under u/pgee/ DM-1659 is now correct. The error I made was to try to define fail() withing the base class ApertureFlux and measure() within the derived class CircularApertureFlux. That resulted in an undefined dynamic call from SWIG, for reasons I don't understand. The checked in version has both methods defined in the derived class. include/lsst/meas/base/ApertureFlux.h | 6 +++--- include/lsst/meas/base/CircularApertureFlux.h | 8 +++++++- python/lsst/meas/base/ApertureFlux.i | 2 +- src/ApertureFlux.cc | 10 ---------- src/CircularApertureFlux.cc | 4 ++++ 5 files changed, 15 insertions , 15 deletions
          Hide
          Permalink
          jbosch Jim Bosch added a comment -

          My only comments are aesthetic:

          • ApertureFlux.h:87: Revert whitespace change (there should be a space before curly bracket open).
          • CircularApertureFlux.h:60: If you want to put the close-paren and "const" on a separate line, it should be indented to match the open paren, or the arguments should be on a separate line from the open paren. But since it fits on one line, I'd recommend just putting it on the same line as the rest of the declaration. (See https://confluence.lsstcorp.org/pages/viewpage.action?pageId=16908737, section 6-6).
          • CircularApertureFlux.cc:60: This line is too long, so it can't go on one line. I'd recommend using the second form in the standards document linked above here, as that's what the rest of the functions in this file use.

          Please amend/rebase/squash these changes into your original commit before merging.

          Show
          jbosch Jim Bosch added a comment - My only comments are aesthetic: ApertureFlux.h:87: Revert whitespace change (there should be a space before curly bracket open). CircularApertureFlux.h:60: If you want to put the close-paren and "const" on a separate line, it should be indented to match the open paren, or the arguments should be on a separate line from the open paren. But since it fits on one line, I'd recommend just putting it on the same line as the rest of the declaration. (See https://confluence.lsstcorp.org/pages/viewpage.action?pageId=16908737 , section 6-6). CircularApertureFlux.cc:60: This line is too long, so it can't go on one line. I'd recommend using the second form in the standards document linked above here, as that's what the rest of the functions in this file use. Please amend/rebase/squash these changes into your original commit before merging.
          Hide
          Permalink
          pgee Perry Gee added a comment -

          I decided that I did not like this solution, so I moved fail() back to ApertureFlux, and found the right combination of declarations to make measure a pure virtual method and fail a virture defined withing ApertureFlux.cc

          Show
          pgee Perry Gee added a comment - I decided that I did not like this solution, so I moved fail() back to ApertureFlux, and found the right combination of declarations to make measure a pure virtual method and fail a virture defined withing ApertureFlux.cc
          Hide
          Permalink
          jbosch Jim Bosch added a comment -

          Yup, I like the new solution better too. But you've now got the "private:" statement in CircularApertureFlux.h between the measure method and its documentation. Other than that, good to merge.

          Show
          jbosch Jim Bosch added a comment - Yup, I like the new solution better too. But you've now got the "private:" statement in CircularApertureFlux.h between the measure method and its documentation. Other than that, good to merge.

            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.