DM-26697: Deprecate needing to pre-pair exposures in PTC task, and calculate an appropriate pair based on header data. #52

plazas · 2020-09-14T18:35:57Z

No description provided.

czwa

I'm concerned with the makePairs method, as it seems like it only will accept two exposures at a given exposure time, and that it may not pair the shortest time interval pair, so there's less guarantee that the illumination will be stable.

czwa · 2020-09-14T21:23:07Z

python/lsst/cp/pipe/ptc.py

@@ -255,9 +254,9 @@ def __setattr__(self, attribute, value):
            self.__dict__[attribute] = value

    def getVisitsUsed(self, ampName):


We're not really using visits anywhere, right? Can we drop it everywhere in favor of 'exposure', or should that be a separate ticket? We'll want to do it at some point.

czwa · 2020-09-14T21:33:29Z

python/lsst/cp/pipe/ptc.py

@@ -446,12 +413,42 @@ def runDataRef(self, dataRef, visitPairs):

            butler.put(linearizer, datasetType='Linearizer', dataId={'detector': detNum,
                       'detectorName': detName, 'calibDate': calibDate})
-
-        self.log.info(f"Writing PTC data to {dataRef.getUri(write=True)}")
+        rerunDir = list(dataRef.getButler().storage.repositoryCfgs.keys())[0]


This seems like a lot of work for something that the butler should be handling. I also don't think that this will work in gen3.

Do we need this line then? {{dataRef.getUri(write=True)}} fails. This line is printing the location of the dataset pickle file. Is there another way of getting this, if we decide that we need to print this information?

I don't think we need to say where we're writing things. A simple "Writing PTC data." will note that it's done with the calculation.

czwa · 2020-09-14T21:44:56Z

python/lsst/cp/pipe/ptc.py

+            expTime = tempFlat.getInfo().getVisitInfo().getExposureTime()
+            listAtExpTime = sortedPairs.setdefault(expTime, [])
+            if len(listAtExpTime) < 2:
+                listAtExpTime.append(tempFlat)


Don't we want to sort on DATE-OBS or SEQNUM or something to prevent multiple flat ramps being paired incorrectly? This also looks like it will toss any extra exposures, even when they could be used as well.

I thought we were assuming for this task that the user would pass flat fields from a single PTC ramp. Are you suggesting that we should check that the DATE-OBS is the same for all exposures? Or do you have something else in mind? What if the flat ramp starts one night and continues to the next day, resulting in the images having different dates?
The way we calculate the PTC now is by taking the difference between two flats at the same expTime to reduce PRNU. If for some reason there are more than two flats for the same expTime, we still need to pick two (the first two, in this code). Do you have something in mind for using possible extra exposures at the same expTime?

I thought it would be general for any number of inputs. The ramps that I'd looked at (from LATISS, though, so maybe different from how they're taking them on BOT) had four exposures at each exposure time. More entries should provide better fits, as we're less dependent on any single pair.
If this is rejecting inputs just because it duplicates an exposure time, then the code should have a log message that something has been dropped. Since this is a user input, I think the expectation is that everything specified on the command line will be used.

I do remember the LATIISS ramps with more than 2 exposures per exposure time, but I think that the PTC, as it's implemented now, uses the difference of only two. I'll add a message as you suggest.

I think dropping exposures is a mistake. This also isn't deterministic: for a set of equal exposure time exposures 1^2^3^4, 1^3^2^4, etc. will all give different outputs. Even if you want to keep only one pair for each exposure time, you need to do some sort operation to ensure that order transposes all give the same result.

I think this is the "date = %Y-%m-%d %H:%M:%S" vs "date = %Y-%m-%d" difference. Sorting along the first (which I think is what getVisitInfo().getDate() returns) should ~~sort~~ resolve this issue.

I don't know, is it? If that is the case, how are you suggesting to use the date of observation to end up with only two flats at the same exposure time? (in case of Nflats with same expTime > 2).

I just saw Chris's comment (after I posted mine, replying to Tim). Thanks!

Yes, VisitInfo stores the full date, not just the day.

This looks right to me. If you can double check that it's deterministic (something like the example above where exposures are specified out of order but still return the in-order sorting), then I think this is done.

timj · 2020-09-14T21:55:53Z

python/lsst/cp/pipe/ptc.py

+
+        Parameters
+        ----------
+        dataReflist : `list` [`lsst.daf.peristence.ButlerDataRef`]


Is there a plan for making this gen3 compatible? (also typo in the class name).

This is step one of that conversion. https://jira.lsstcorp.org/browse/DM-21786 is the umbrella ticket for this.

Thanks for pointing out the typo. Regarding gen3 compatibility, I'm not sure about what you are referring to. Are you talking about the "makePairs" function in particular? Or the PTC task in general? If the latter, yes, this ticket is part of a larger ticket (DM-21786) to convert the PTC task to gen3.

I was just noting that this is brand new code that will have to be rewritten on a November timeline.

The idea is that after DM-21786 the PTC task will be fully converted to gen3.

timj · 2020-09-14T22:00:22Z

python/lsst/cp/pipe/ptc.py

+
+        Return
+        ------
+        sortedPairs : `dict`


I think you need to say dict of what? Keys are exposure time in seconds and values are list of Exposure? But only the first two exposures that have that same exposure time? And what is sorting what? I don't see any sorting at all.

Yes, I agree, I'll add more information about the dictionary: the keys and values are as you describe. The input exposures are not necessarily sorted by ascending time in the command line, so this function organizes them in a dictionary. I can change the name sorted to something else (e.g., just "pairs").

I definitely would not use sorted anywhere in the function if nothing is doing any sorting.

Fix typo

czwa · 2020-09-16T19:46:58Z

python/lsst/cp/pipe/ptc.py

+            expTime = tempFlat.getInfo().getVisitInfo().getExposureTime()
+            listAtExpTime = sortedPairs.setdefault(expTime, [])
+            if len(listAtExpTime) < 2:
+                listAtExpTime.append(tempFlat)


This looks right to me. If you can double check that it's deterministic (something like the example above where exposures are specified out of order but still return the in-order sorting), then I think this is done.

Deprecate the need of input flat pairs

1ba0964

plazas requested a review from czwa September 14, 2020 18:35

czwa requested changes Sep 14, 2020

View reviewed changes

timj reviewed Sep 14, 2020

View reviewed changes

plazas added 6 commits September 15, 2020 11:07

Add docstrings and report rerun dir with dataset

af40d18

Fix typo

Replace 'visit' for 'expId'

2e21756

Simplify info message about writing PTC data

8f3af17

Replace 'visit' with 'expId' in tests

63f7778

Rename dict in makePairs and print info msgs

2f0d23c

Add more 'visit' to 'expId' replacements

db2bbfd

plazas force-pushed the tickets/DM-26697 branch from f68bd1a to db2bbfd Compare September 15, 2020 16:08

plazas added 2 commits September 15, 2020 11:44

Print expIds used in PTC

bca5158

Organize exposures by obs date

d10a53b

czwa approved these changes Sep 16, 2020

View reviewed changes

plazas merged commit 4cba61e into master Sep 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-26697: Deprecate needing to pre-pair exposures in PTC task, and calculate an appropriate pair based on header data. #52

DM-26697: Deprecate needing to pre-pair exposures in PTC task, and calculate an appropriate pair based on header data. #52

plazas commented Sep 14, 2020

czwa left a comment

czwa Sep 14, 2020

czwa Sep 14, 2020

plazas Sep 14, 2020

czwa Sep 14, 2020

czwa Sep 14, 2020

plazas Sep 14, 2020

czwa Sep 14, 2020 •

edited

plazas Sep 15, 2020

czwa Sep 15, 2020

czwa Sep 15, 2020 •

edited

plazas Sep 15, 2020

plazas Sep 15, 2020

timj Sep 15, 2020

czwa Sep 16, 2020

timj Sep 14, 2020

czwa Sep 14, 2020

plazas Sep 14, 2020

timj Sep 14, 2020

plazas Sep 15, 2020

timj Sep 14, 2020 •

edited

plazas Sep 14, 2020

timj Sep 14, 2020

czwa Sep 16, 2020

		@@ -255,9 +254,9 @@ def __setattr__(self, attribute, value):
		self.__dict__[attribute] = value

		def getVisitsUsed(self, ampName):

DM-26697: Deprecate needing to pre-pair exposures in PTC task, and calculate an appropriate pair based on header data. #52

DM-26697: Deprecate needing to pre-pair exposures in PTC task, and calculate an appropriate pair based on header data. #52

Conversation

plazas commented Sep 14, 2020

czwa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czwa Sep 14, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czwa Sep 15, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timj Sep 14, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czwa Sep 14, 2020 •

edited

czwa Sep 15, 2020 •

edited

timj Sep 14, 2020 •

edited