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

Should only read fringe data after checking the filter

    XMLWordPrintable

    Details

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

      Description

      The fringe subtraction is not necessarily performed if doFringe is True. It is only if the filter of the raw exposure is listed in config fringe.filters.

      Fringe data should not be read unless the filter is indicated. There are likely no such filter data and it would cause runtime errors.

      Seems related to changes from RFC-26 and DM-1299.

        Attachments

          Issue Links

            Activity

            Hide
            yusra Yusra AlSayyad added a comment -

            That is fair. From an users perspective, I can imagine thinking "I already said config.doFringe=False. Why do I also have to say getFringeData=False?" I'm thinking that your initial commit that passed the whole ccdExposure may be better than getFringeData without a default (especially if it makes your edits to the obs_ packages cleaner). I guess it comes down to the lesser of two evils: require the users to pass an exposure just to get some data from the butler or ask the users to decide whether to read in the fringe data.

            Hopefully, New Butler will allow us to write a def checkFilters(self, sensorRef). Seems like it should be possible if the butler can already find the flats/fringes of the correct filter based only on the dataId.

            Show
            yusra Yusra AlSayyad added a comment - That is fair. From an users perspective, I can imagine thinking "I already said config.doFringe=False. Why do I also have to say getFringeData=False?" I'm thinking that your initial commit that passed the whole ccdExposure may be better than getFringeData without a default (especially if it makes your edits to the obs_ packages cleaner). I guess it comes down to the lesser of two evils: require the users to pass an exposure just to get some data from the butler or ask the users to decide whether to read in the fringe data. Hopefully, New Butler will allow us to write a def checkFilters(self, sensorRef) . Seems like it should be possible if the butler can already find the flats/fringes of the correct filter based only on the dataId.
            Hide
            hchiang2 Hsin-Fang Chiang added a comment -

            Alright, branch u/hfc-3/DM-4266 in ip_isr and tickets/DM-4266 in obs_lsstSim have the latest corrections. No other obs_* packages call readIsrData.

            The current solution is that the raw exposure is passed to readIsrData, and there is no default. Hopefully the comment of the argument is clear that it won't encourage misuse.

            For now FringeTask.readFringes still returns a Struct including fringes, I am still thinking to refactor so that it returns fringes only, probably in a future ticket. It was discussed a little bit in HipChat (Nov 12, Science Pipelines room) and my notes from there:

            • It is very important to be entirely deterministic
            • The seed should be different for every exposure (fringe exposure?)
            • Would be even better if the fringe regions and fringe measurements are persisted
            Show
            hchiang2 Hsin-Fang Chiang added a comment - Alright, branch u/hfc-3/ DM-4266 in ip_isr and tickets/ DM-4266 in obs_lsstSim have the latest corrections. No other obs_* packages call readIsrData . The current solution is that the raw exposure is passed to readIsrData , and there is no default. Hopefully the comment of the argument is clear that it won't encourage misuse. For now FringeTask.readFringes still returns a Struct including fringes , I am still thinking to refactor so that it returns fringes only, probably in a future ticket. It was discussed a little bit in HipChat (Nov 12, Science Pipelines room) and my notes from there: It is very important to be entirely deterministic The seed should be different for every exposure (fringe exposure?) Would be even better if the fringe regions and fringe measurements are persisted
            Hide
            yusra Yusra AlSayyad added a comment -

            OK to merge after it passes Jenkins or buildbot test. Remember that the builds don't run all the obs_ packages, so check by hand that obs_cfht unit tests pass.

            Unrelated note: I see lots of draft branches for this ticket. The project encourages rebasing if you're working alone on a branch. I know I'll edit my commits many many times between creating the branch and merging.

            Show
            yusra Yusra AlSayyad added a comment - OK to merge after it passes Jenkins or buildbot test. Remember that the builds don't run all the obs_ packages, so check by hand that obs_cfht unit tests pass. Unrelated note: I see lots of draft branches for this ticket. The project encourages rebasing if you're working alone on a branch. I know I'll edit my commits many many times between creating the branch and merging.
            Show
            hchiang2 Hsin-Fang Chiang added a comment - Passed Jenkins: https://ci.lsst.codes/job/stack-os-matrix/label=centos-6/5672//console
            Hide
            hchiang2 Hsin-Fang Chiang added a comment -

            Tests of obs_lsstSim and ip_isr are included in the Jenkins build.
            Also ran and passed obs_cfht and obs_decam tests manually. Do not have the correct testdata_subaru to run obs_subaru tests.

            Changes are merged to master.

             python/lsst/obs/lsstSim/lsstSimIsrTask.py | 2 +-
             tests/testLsstSimIsrTask.py               | 2 +-
             2 files changed, 2 insertions(+), 2 deletions(-)
             
             python/lsst/ip/isr/isrTask.py | 24 +++++++++++++++---------
             1 file changed, 15 insertions(+), 9 deletions(-)
            

            Show
            hchiang2 Hsin-Fang Chiang added a comment - Tests of obs_lsstSim and ip_isr are included in the Jenkins build. Also ran and passed obs_cfht and obs_decam tests manually. Do not have the correct testdata_subaru to run obs_subaru tests. Changes are merged to master. python/lsst/obs/lsstSim/lsstSimIsrTask.py | 2 +- tests/testLsstSimIsrTask.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)   python/lsst/ip/isr/isrTask.py | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-)

              People

              Assignee:
              hchiang2 Hsin-Fang Chiang
              Reporter:
              hchiang2 Hsin-Fang Chiang
              Reviewers:
              Yusra AlSayyad
              Watchers:
              Hsin-Fang Chiang, Yusra AlSayyad
              Votes:
              0 Vote for this issue
              Watchers:
              2 Start watching this issue

                Dates

                Created:
                Updated:
                Resolved:

                  Jenkins

                  No builds found.