DM-32670: Running ISR and PTC task on BOT data on 189 CCDs fails at NCSA #113
Conversation
| @@ -240,7 +244,8 @@ def run(self, inputExp, inputDims): | |||
| """ | |||
| # inputExp.values() returns a view, which we turn into a list. We then | |||
| # access the first exposure-ID tuple to get the detector. | |||
| detector = list(inputExp.values())[0][0][0].getDetector() | |||
| # The first "get()" retrieves the exposure from the exposure reference. | |||
| detector = list(inputExp.values())[0][0][0].get().getDetector() | |||
There was a problem hiding this comment.
I wonder if this results in getting the whole first exposure twice (once here, and once later when it's used) or if that's smart enough to just get the detector. I suspect it's not that smart though. But I also am not convinced it's worth the extra code complexity for special-casing the first exp, given the overhead, and how often this is called, so I guess leave as-is.
| exp2, expId2 = exposures[1] | ||
| expRef1, expId1 = exposures[0] | ||
| expRef2, expId2 = exposures[1] | ||
| # use get() to obtain `lsst.afw.image.Exposure` |
There was a problem hiding this comment.
I am not sure comments for things that are so self-describing are worth it tbh, but up to you.
python/lsst/cp/pipe/utils.py
Outdated
| (and their IDs) that have | ||
| the same exposure time (seconds). |
There was a problem hiding this comment.
Maybe put this line on the line above now it's short enough to be combined.
| for exp, expId in zip(exposureList, exposureIdList): | ||
| expTime = exp.getInfo().getVisitInfo().getExposureTime() | ||
| for expRef, expId in zip(exposureList, exposureIdList): | ||
| expTime = expRef.get(component='visitInfo').exposureTime |
There was a problem hiding this comment.
Actually, given you can do this, now I do wonder if you can use the component='detector' for my first comment.
There was a problem hiding this comment.
It is much faster to get just the detector via the butler if you don't want the entire exposure read into memory.
There was a problem hiding this comment.
Merlin: I woudl still need to get the exposure from the reference first, so it would not be much different to what's already above: detector=list(inputExp.values())[0][0][0].get().getDetector() vs list(inputExp.values())[0][0][0].get(component='detector') (if it works)
Tim: In this function in utils we don't need the detector. Are you referring to detector=list(inputExp.values())[0][0][0].get().getDetector() in the run method:
run function as well from runQuantum? I wanted to just use the inputs to run: inputExp or inputDims
There was a problem hiding this comment.
I was replying to Merlin's comment about component get vs Exposure get then getComponent -- of course if you need the Exposure then you should get the component from the exposure.
There was a problem hiding this comment.
Right, the point being that you do need to get it, but at the moment you're getting the whole thing twice, and so changing the instance above (my first comment on the PR) would mean that the first time you don't also retrieve the image. Given there's a nice syntax for it, and it doesn't involve having to get the whole thing and then special case the first pass through this loop (which is what I was worrying about) it like is worth it after all, I'd imagine (though perhaps given caching, it really isn't goig to matter that much in practice).
There was a problem hiding this comment.
I see, thanks. I switched to using get(component='detector') above.
plazas
force-pushed
the
tickets/DM-32670
branch
2 times, most recently
from
e680374
to
f5935d8
Compare
plazas
force-pushed
the
tickets/DM-32670
branch
from
f5935d8
to
45d81cd
Compare
No description provided.