# Should only read fringe data after checking the filter

XMLWordPrintable

#### Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
None
• Story Points:
8
• Team:
Data Facility

#### 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.

#### Activity

Hide

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 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
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
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

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 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.
Hide
Hsin-Fang Chiang added a comment -
Show
Hsin-Fang Chiang added a comment - Passed Jenkins: https://ci.lsst.codes/job/stack-os-matrix/label=centos-6/5672//console
Hide
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
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:
Hsin-Fang Chiang
Reporter:
Hsin-Fang Chiang
Reviewers:
Watchers: