DM-35207: use updated visit summaries in downstream tasks #741
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
TallJimbo
changed the title
TallJimbo
force-pushed
the
tickets/DM-35207
branch
7 times, most recently
from
dfb2d55
to
e1f2949
Compare
I'm sure this is just the tip of the iceberg, but I noticed it and might as well fix it.
This includes adjusting some docs that were a bit too detailed about which task would produce this input.
These are all tasks that aren't usefully affected by fake insertion (at most, rerunning them would change summary stats by including measurements of fakes that were inserted with the same PSFs, WCS, etc. that we're trying to summarize, which is just circular). Unfortunately, we can't actually drop the calexpType connection template, because that's a config field and those need to be deprecated to avoid breakage when reading old configs. That'll have to wait for DM-37465.
The finalVisitSummary datasets should have the same objects as the finalized_psf_ap_corr_catalog dataset, and using them here instead will make it possible to delete the latter earlier and reduce the number of compute nodes we ever have to transfer it to.
The finalVisitSummary datasets should have the same objects as the direct FGCM output datasets, and using them here instead will make it possible to delete the latter earlier and reduce the number of compute nodes we ever have to transfer them to.
The finalVisitSummary datasets hold the best-per-detector-over-tracts outputs from jointcal (if jointcal is being run), providing more consistency and making it possible to delete the jointcal outputs earlier in the processing.
TallJimbo
force-pushed
the
tickets/DM-35207
branch
from
805c2c8
to
6401943
Compare
laurenam
approved these changes
Jan 9, 2023
| @@ -98,7 +98,7 @@ class CoaddBaseConfig(pexConfig.Config): | |||
| ) | |||
| useGlobalExternalSkyWcs = pexConfig.Field( | |||
| dtype=bool, | |||
| default=False, | |||
| default=True, | |||
There was a problem hiding this comment.
Is it a bit of a falsehood to say we're using the global versions (we don't actually have any, do we?!).
| image_mask : `lsst.afw.image.Mask`, optional | ||
| Mask image that may be used to compute distance-to-nearest-star | ||
| metrics. | ||
| sources_columns : `collections.abc.Set` [ `str` ], optional | ||
| sources_is_astropy : `bool`, optional |
There was a problem hiding this comment.
This docstring needs updating.
| sources_columns = sources.schema.getNames() | ||
|
|
There was a problem hiding this comment.
Do we still think returning silently below is ok? I think at least a log message would be very useful.
[Ah...forget it...just looked at the next commit!]
tests/test_isPrimaryFlag.py
Outdated
| @@ -161,6 +161,7 @@ def setUp(self): | |||
|
|
|||
| # Characterize the image (create PSF, etc.) | |||
| charImConfig = CharacterizeImageConfig() | |||
| charImConfig.doComputeSummaryStats = False | |||
There was a problem hiding this comment.
This is now a no-op in CharacterizeImage (funny that the default was left to True there 🤪)
...with a slightly different interface. This compatibility was added on DM-37412 with the intent that it'd be used by this ticket (DM-35207), but it was broken on DM-37411 in the interim, and now needs to work slightly differently (and be more specific about astropy being the other option).
The check removed here was apparently in place to allow ComputeSummaryStatsTask to be run in some tests that aren't about the summary stats run it anyway, under artificial conditions. But that check could also turn serious configuration errors into silent failures at production. It's better to not run this task in those tests.
As we've learned while testing this branch, isinfinite combined with shortcircuiting "or" can lead to a false sense of security.
TallJimbo
force-pushed
the
tickets/DM-35207
branch
from
3ff480c
to
c66ba93
Compare
TallJimbo
force-pushed
the
tickets/DM-35207
branch
2 times, most recently
from
8d9a465
to
c66ba93
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.