-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DM-48932: Add new tasks to run finalize characterization in parallel over detectors. #1051
Conversation
a980a9f
to
db005ba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Some minor questions/comments.
Processing summary | ||
================== | ||
|
||
`FinalizeCharacterizationDetectorTask` first reads the isolated source association table to get a consistent list of PSF modeling and reserve stars, and then runs PSF modeling and aperture correction modeling per detector. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consistent in regards to what?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overlapping visits. Updated.
- Run the aperture correction measurement task configured in ``config.measure_ap_corr`` using the PSF stars. | ||
- Run the aperture correction application task configured in ``config.apply_ap_corr`` on the PSF stars. | ||
|
||
An exposure catalog containing the PSF model for the detector and the aperture correction map, and a an astropy table with all the measurements and flags are returned for persistence. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a an
-> an
. Also suggest rewriting as The task returns...
@@ -30,7 +30,7 @@ In the second stage, the task will run per-detector: | |||
- Run the aperture correction measurement task configured in ``config.measure_ap_corr`` using the PSF stars. | |||
- Run the aperture correction application task configured in ``config.apply_ap_corr`` on the PSF stars. | |||
|
|||
An exposure catalog containing all the PSF models in the visit, another containing all the aperture correction maps in the visit, and a data frame with all the measurements and flags are returned for persistence. | |||
An exposure catalog containing all the PSF models in the visit and all the aperture correction maps in the visit, and an astropy table with all the measurements and flags are returned for persistence. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also suggest The task returns...
if you make that change above.
defaultTemplates={}, | ||
): | ||
src = pipeBase.connectionTypes.Input( | ||
doc='Source catalogs for the visit', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Source catalog (for the detector?)
dimensions=('instrument', 'visit', 'detector'), | ||
) | ||
calexp = pipeBase.connectionTypes.Input( | ||
doc='Calexps for the visit', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calexp
outputRefs.finalized_psf_ap_corr_cat) | ||
butlerQC.put(astropy.table.Table(struct.output_table), | ||
outputRefs.finalized_src_table) | ||
|
||
def run(self, visit, band, isolated_star_cat_dict, isolated_star_source_dict, src_dict, calexp_dict): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh... can you actually run this task, in practice? What would it do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha! This should have been deleted.
|
||
if len(measured_src_tables) > 0: | ||
measured_src_table = np.concatenate(measured_src_tables) | ||
measured_src_table = astropy.table.vstack(measured_src_tables, join_type='exact') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not TableVStack? Or these are already in memory so there's no need for it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are in memory, so there's no need.
Raised if the selector returns no good sources. | ||
""" | ||
# We do not need the isolated star table in this task. | ||
# However, it is used in tests to confirm consistency of indexes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like it should be removed as a connection then, although if it's not worth the effort then fair enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to upset the apple cart too much here...
) | ||
finalized_psf_ap_corr_cat = pipeBase.connectionTypes.Output( | ||
doc=('Per-visit finalized psf models and aperture corrections. This ' | ||
'catalog uses detector id for the id and are sorted for fast ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ids are sorted/the catalog is sorted? Same difference I suppose, but isn't there still a source id as primary key even if it's not sorted by it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is wording that John P. and I came up with ages ago, all the catalogs have it. And if it isn't sorted then the lookup is slow. So "sorted for fast" is the operative phrase.
db005ba
to
51b53cb
Compare
No description provided.