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-46327: Exclude FGCM tasks from LSSTCam/LSSTComCam #155

Merged
merged 2 commits into from
Sep 18, 2024

Conversation

leeskelvin
Copy link
Contributor

No description provided.

Copy link
Contributor

@cmsaunders cmsaunders left a comment

Choose a reason for hiding this comment

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

This mostly looks good. I think a few things could be moved to obs_lsst. The only thing that definitely needs to be changed is the astrometricRefCatPreSourceVisit input name. Let me know if I can help with figuring out the error that led to that change.

@@ -75,7 +75,8 @@ tasks:
class: lsst.analysis.tools.tasks.refCatSourceAnalysis.RefCatSourceAnalysisTask
config:
# Only run metrics for analyzing the preSources:
connections.data: preSourceTable_visit_gaia_dr3_20230707_match_astrom
connections.data: preSourceTable_visit_uw_stars_20240524_match_astrom
Copy link
Contributor

Choose a reason for hiding this comment

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

I think what was already here was correct. The uw_stars... catalog is only for ComCamSim. (The outputName below looks correct however.)

Copy link
Contributor Author

@leeskelvin leeskelvin Sep 18, 2024

Choose a reason for hiding this comment

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

I think this was inadvertently changed in an effort to diagnose the cascade of errors, comparing against LSSTComCamSim. Changed back, here and for LSSTComCam.

@@ -75,7 +75,8 @@ tasks:
class: lsst.analysis.tools.tasks.refCatSourceAnalysis.RefCatSourceAnalysisTask
config:
# Only run metrics for analyzing the preSources:
connections.data: preSourceTable_visit_gaia_dr3_20230707_match_astrom
connections.data: preSourceTable_visit_uw_stars_20240524_match_astrom
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as LSSTCam comment, what was here before should be correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed back to preSourceTable_visit_gaia_dr3_20230707_match_astrom.

photometricMatchVisit:
class: lsst.analysis.tools.tasks.photometricCatalogMatch.PhotometricCatalogMatchVisitTask
config:
extraColumns: ["x", "y", "ap09Flux", "ap09FluxErr"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these have to go here because they're not present when photometricMatchPreVisit is run and so would cause that task to fail if they were in obs_subaru?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you're referring to the extraColumns, I have no opinion on those. They were simply copied in verbatim from $ANALYSIS_TOOLS_DIR/pipelines/visitQualityCore.yaml, a YAML which gets imported into this YAML. That makes photometricMatchVisit visible in this YAML. I needed to over-ride the referenceCatalogLoader.doApplyColorTerms config to false, which necessitates me redefining the entire task and prior configs again here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought this was the case, but in a recent slack thread there was a big discussion about how the configs are actually supposed to be inherited.

class: lsst.analysis.tools.tasks.photometricCatalogMatch.PhotometricCatalogMatchVisitTask
config:
extraColumns: ["x", "y", "ap09Flux", "ap09FluxErr"]
referenceCatalogLoader.doApplyColorTerms: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make this change in obs_lsst like you do for photometricCatalogMatchTask?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's much neater, thank you.

@@ -116,6 +122,7 @@ tasks:
connections.catalog: preSourceTable_visit
connections.targetCatalog: preSourceTable_visit
connections.matchedCatalog: preSourceTable_visit_gaia_dr3_20230707_photoMatch
referenceCatalogLoader.doApplyColorTerms: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, can't you set this in obs_lsst?

class: lsst.analysis.tools.tasks.photometricCatalogMatch.PhotometricCatalogMatchVisitTask
config:
extraColumns: ["x", "y", "ap09Flux", "ap09FluxErr"]
referenceCatalogLoader.doApplyColorTerms: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, can you set this in obs_lsst?

@leeskelvin
Copy link
Contributor Author

Thanks @cmsaunders, configs have been moved to obs_lsst where appropriate, significantly cleaning up this PR and removing 1 commit.

@leeskelvin leeskelvin merged commit 5946703 into main Sep 18, 2024
3 checks passed
@leeskelvin leeskelvin deleted the tickets/DM-46327 branch September 18, 2024 23:16
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.

2 participants