Review of DM-4373 (HSC backport: Add tract conveniences) #27
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -3,6 +3,7 @@ setupRequired(pipe_base) | |||
| setupRequired(daf_base) | |||
| setupRequired(pex_config) | |||
| setupRequired(coadd_utils) | |||
| setupOptional(geom) | |||
There was a problem hiding this comment.
I'm a bit confused as to how this can be an optional dependency if there is an unprotected import in the code above.
There was a problem hiding this comment.
Sorry, I blindly followed the HSC entry (which I guess should have been Required). Amended.
We want to iterate over all tracts spanned by the data when no tract is provided (the user doesn't want to have to look up what tracts are spanned). This patch determines the set of tracts each visit touches and adds a data reference with those tracts for each of the visit components. This was originally implemented for meas_mosaic, but moved here to be of more general use, and applied to forcedPhotCcd. Conflicts: python/lsst/pipe/tasks/forcedPhotCcd.py This is effectively a cherry-pick of HSC-715's commit: HyperSuprime-Cam/pipe_tasks@d0e5a1a
Conflicts: python/lsst/pipe/tasks/forcedPhotCcd.py This is effectively a cherry-pick of HSC-715's commit: HyperSuprime-Cam/pipe_tasks@eb1ca20
Modify PerTractCcdDataIdContainerto inherit from lsst.pipe.base.DataIdContainer instead of CoaddDataIdContainer. Inheriting from the latter was confusing as it pertains to Coadds and this method pertains to Ccds. Additionally, the only functionality it was gaining was the setting and memoising of the skymap via the getSkymap() function. The memoizing was in fact also being done in the function itself, so simply set the skymap explicitly there (thus omiting the need to call getSkymap() and its inheritance from from CoaddDataIdContainer).
The override version of castDataIds() was implemented to add the tract/int idKey/idKeyType to the idKeyTypeDict. However, this is not necessary as this entry exists in the latter regardless of whether the user supplies the tract in the data id on the command line.
laurenam
force-pushed
the
u/lauren/DM-4373
branch
from
6d7a8b9
to
6ec5c11
Compare
| continue | ||
|
|
||
| visit = ref.dataId["visit"] | ||
| visitRefs = collections.defaultdict(list) |
There was a problem hiding this comment.
This looks wrong: every time around the loop you're replacing whatever visitRefs used to refer to with a new, empty defaultdict. You probably want to move this line outside the loop, replacing line 71.
Includes: - update copyright - whitespace - make use of defaultdict - remove conditional setting of lsst.pex.logging for clarity - update help message in argument parser
Currently, forcedPhotCcd.py is the only place where the PerTractCcdDataIdContainer is being used, so it seems appropriate just to include it there (as opposed to having in its own file). The only other place currently planned to make use of it is in meas_mosaic, which is not yet implemented. If a more suitable location for this container becomes apparent (e.g. in the process of getting meas_mosaic running with the stack), it can be moved accordingly then.
laurenam
force-pushed
the
u/lauren/DM-4373
branch
from
eeceef3
to
4410f1c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.