Skip to content
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-33279: Add IsolatedStarAssociationTask #633

Merged
merged 9 commits into from Feb 23, 2022
Merged

DM-33279: Add IsolatedStarAssociationTask #633

merged 9 commits into from Feb 23, 2022

Conversation

erykoff
Copy link
Contributor

@erykoff erykoff commented Feb 14, 2022

This PR adds a new isolated star association task for input to re-estimating the psf with consistently reserved stars, and for input to fgcmcal.

Note that the Matcher code currently in the commit is just there for testing purposes until DM-33545 is finished and we have a new env with spherematch.

Copy link
Contributor

@fred3m fred3m left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks really good! I just have a few comments and suggests for cleaning things up a bit.

python/lsst/pipe/tasks/isolatedStarAssociation.py Outdated Show resolved Hide resolved
python/lsst/pipe/tasks/isolatedStarAssociation.py Outdated Show resolved Hide resolved
python/lsst/pipe/tasks/isolatedStarAssociation.py Outdated Show resolved Hide resolved
python/lsst/pipe/tasks/isolatedStarAssociation.py Outdated Show resolved Hide resolved
python/lsst/pipe/tasks/isolatedStarAssociation.py Outdated Show resolved Hide resolved
python/lsst/pipe/tasks/isolatedStarAssociation.py Outdated Show resolved Hide resolved
python/lsst/pipe/tasks/isolatedStarAssociation.py Outdated Show resolved Hide resolved
Copy link
Contributor

@parejkoj parejkoj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the docs: the processing summary there is very helpful.

A few wording comments, and one potentially bigger one about the term "observation".

Processing summary
==================

``IsolatedStarAssociationTask`` reads in all the visit-level source table parquet catalogs in a given tract and matches (associates) the observations together.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth linking to the ConsolidateSourceTableTask or something here? We sadly don't have actual datasetType docs yet, nor a way to link to them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how to do that, and since there's no guidance to how to do the dataset linkages I'd prefer to let it go for now...

###########################

``IsolatedStarAssociationTask`` matches unresolved stars from the ``sourceTable_visit`` datasets into a pure (but not complete) catalog of isolated stars.
The output datasets are a catalog of isolated stars (dataset ``isolated_star_cat``) and the associated observations with configured columns from the input catalogs (dataset ``isolated_star_observations``).
Copy link
Contributor

@parejkoj parejkoj Feb 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"and the associated observations" -> "and a catalog of properties from each of the associated sources"? Trying to keep object/source terminology (since I think we use "source" where you're using "observation" here)? Which maybe suggests isolated_star_observations should be isolated_star_sources, or maybe I'm misunderstanding something.

I think "observation" in Rubin typically implies properties about the exposure itself, not the sources?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're right, and that's a good catch before things get embedded in the system.

- Unflagged, unresolved stars above a configured signal-to-noise are selected from each input catalog.
- The input stars are associated within a configured ``match_radius``.
This association is done band-by-band (with the order configured by ``band_order``), and then each band is matched such that all observations are associated with a unique list of stars which cover all the bands in the tract.
- The star catalog is matched against itself and group of stars that matches neighbor(s) within ``isolation_radius`` is removed from the final isolated star catalog (``isolated_star_cat``).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"and group of stars that matches" - I think something's not right with the grammar here, but I can't figure out what.

@erykoff erykoff merged commit a959e0c into main Feb 23, 2022
@erykoff erykoff deleted the tickets/DM-33279 branch February 23, 2022 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants