Uploaded image for project: 'Data Management'
  1. Data Management
  2. DM-12366 Add spatially varying transmissions curves and coadd them
  3. DM-12373

Add spatially-varying transmission curves to Exposure/ExposureRecord

    Details

    • Type: Technical task
    • Status: Done
    • Resolution: Done
    • Fix Version/s: None
    • Component/s: afw
    • Labels:
    • Story Points:
      3
    • Team:
      External

      Description

      Attach an instance of the ABC defined in DM-12367 to Exposure and ExposureRecord.

      While we could add these to Filter rather than add it to Exposure directly, my preliminary feeling is that it'll be better to let this represent the full throughput rather than associating it strictly with the filter. It may also make sense to make Filter hold one of these objects to represent its contribution only, but that isn't what's needed for DM-12366.

        Attachments

          Activity

          Hide
          jbosch Jim Bosch added a comment - - edited

          Krzysztof Findeisen, do you think you could find time for a moderately large mostly-C++ review either tomorrow or early next week?

          The work on this ticket actually encompasses several of the sibling subtasks of this issue - it turned out to be easiest to do a lot of them on the same branch. I haven't yet finished all of the sibling subtasks, but the code here (the parts that will go in afw) were enough to be tested and merged separately, and it'll be helpful to get that part on master (and into a weekly) while working on the remainder.

          All code is in afw, and it's primarily a new C++ abstract base class, TransmissionCurve, and several private subclasses defined in its source file and exposed via factory functions. The new object is also included in ExposureInfo and ExposureRecord, and I had to add a bit of functionality to the afw::table::io::Peristable framework in order to make one of the subclasses persistable. There are also a few cleanup commits on old "nearby" code, so it would probably be easiest to review commit-by-commit instead of looking at the whole changeset.

          Final, please note that the PR is a diff against tickets/DM-12740, which is a prerequisite that I've not yet merged to master (I'm hoping to run Jenkins before doing so, and it's still down due to NCSA maintenance).

          Show
          jbosch Jim Bosch added a comment - - edited Krzysztof Findeisen , do you think you could find time for a moderately large mostly-C++ review either tomorrow or early next week? The work on this ticket actually encompasses several of the sibling subtasks of this issue - it turned out to be easiest to do a lot of them on the same branch. I haven't yet finished all of the sibling subtasks, but the code here (the parts that will go in afw) were enough to be tested and merged separately, and it'll be helpful to get that part on master (and into a weekly) while working on the remainder. All code is in afw, and it's primarily a new C++ abstract base class, TransmissionCurve , and several private subclasses defined in its source file and exposed via factory functions. The new object is also included in ExposureInfo and ExposureRecord , and I had to add a bit of functionality to the afw::table::io::Peristable framework in order to make one of the subclasses persistable. There are also a few cleanup commits on old "nearby" code, so it would probably be easiest to review commit-by-commit instead of looking at the whole changeset. Final, please note that the PR is a diff against tickets/ DM-12740 , which is a prerequisite that I've not yet merged to master (I'm hoping to run Jenkins before doing so, and it's still down due to NCSA maintenance).
          Hide
          krzys Krzysztof Findeisen added a comment -

          I can try, but 2000 lines is a rather hefty changeset. It might take me most of the week to look at everything.

          Show
          krzys Krzysztof Findeisen added a comment - I can try, but 2000 lines is a rather hefty changeset. It might take me most of the week to look at everything.
          Hide
          jbosch Jim Bosch added a comment -

          Understood. Thanks, and hopefully it won't be as bad as it seems; there's a fair bit of boilerplate repetition.

          Show
          jbosch Jim Bosch added a comment - Understood. Thanks, and hopefully it won't be as bad as it seems; there's a fair bit of boilerplate repetition.
          Hide
          krzys Krzysztof Findeisen added a comment -

          Review complete; I'd like to hear your thoughts on exception safety and code duplication before I mark it as reviewed.

          Show
          krzys Krzysztof Findeisen added a comment - Review complete; I'd like to hear your thoughts on exception safety and code duplication before I mark it as reviewed.
          Hide
          jbosch Jim Bosch added a comment -

          Jenkins revealed a problem (down in ci_ctio09m!) with VisitInfo that was apparently (mysteriously!) due to my "Cleanup ExposureInfo constructor" commit. I've removed it from the ticket branch and archived it on tickets/DM-12373b. I think it's one of those pybind11 circular dependencies bugs, so hopefully we can bring this commit back (or perhaps do a bigger cleanup) when we've fixed the way we organize our pybind11 source. For now, it's not necessary, and that code is clearly extremely fragile.

          Show
          jbosch Jim Bosch added a comment - Jenkins revealed a problem (down in ci_ctio09m !) with VisitInfo that was apparently (mysteriously!) due to my "Cleanup ExposureInfo constructor" commit. I've removed it from the ticket branch and archived it on tickets/ DM-12373 b. I think it's one of those pybind11 circular dependencies bugs, so hopefully we can bring this commit back (or perhaps do a bigger cleanup) when we've fixed the way we organize our pybind11 source. For now, it's not necessary, and that code is clearly extremely fragile.
          Hide
          jbosch Jim Bosch added a comment -

          This was not actually merged; I ignored my own note that the GitHub PR was not against master and hence the button there couldn't be used. I have rebased on the latest master and am re-running Jenkins to make sure nothing has broken.

          Show
          jbosch Jim Bosch added a comment - This was not actually merged; I ignored my own note that the GitHub PR was not against master and hence the button there couldn't be used. I have rebased on the latest master and am re-running Jenkins to make sure nothing has broken.
          Hide
          jbosch Jim Bosch added a comment -

          Finally actually merged to master.

          Show
          jbosch Jim Bosch added a comment - Finally actually merged to master.

            People

            • Assignee:
              jbosch Jim Bosch
              Reporter:
              jbosch Jim Bosch
              Reviewers:
              Krzysztof Findeisen
              Watchers:
              Ian Sullivan, Jim Bosch, Krzysztof Findeisen
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: