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-43711: Add contracts for analysis_tools plots #175

Merged
merged 1 commit into from Apr 5, 2024
Merged

Conversation

isullivan
Copy link
Contributor

Note that most analysis_tools Tasks require the input connections to be named 'data'

Copy link
Member

@kfindeisen kfindeisen left a comment

Choose a reason for hiding this comment

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

Contract itself looks good, but please keep the file in order so that it's easy to find later.

@@ -135,3 +135,6 @@ contracts:
- contract: transformDiaSrcCat.connections.ConnectionsClass(config=transformDiaSrcCat).diaSourceTable.name ==
diaPipe.connections.ConnectionsClass(config=diaPipe).diaSourceTable.name
msg: "transformDiaSrcCat.diaSourceTable != diaPipe.diaSourceTable"
- contract: detectAndMeasure.connections.ConnectionsClass(config=detectAndMeasure).spatiallySampledMetrics.name ==
diffimTaskPlots.connections.ConnectionsClass(config=diffimTaskPlots).data.name
msg: "detectAndMeasure.spatiallySampledMetrics != diffimTaskPlots.spatiallySampledMetrics"
Copy link
Member

Choose a reason for hiding this comment

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

So that people don't get lost when making future edits, please keep the contracts sorted as described in the comment. In this case, it would go with the other detectAndMeasure contracts, probably at the end (though diffimTaskPlots subverts the assumption of a linear pipeline...).

Copy link
Member

Choose a reason for hiding this comment

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

Can you also add contracts for analyzeAssocDiaSrcCore, analyzeTrailedDiaSrcCore, and diffimTaskCore? The same bug could potentially happen there.

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, though diffimTaskCore is weird because it reads the task metadata instead of the output data products. That does not get written with any of our parameters like fakesType or coaddName.

Copy link
Member

Choose a reason for hiding this comment

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

Good point, a contract on the metadata would be redundant with the task labels already imposed by the pipeline.

Note that most analysis_tools Tasks require the input connections to be named 'data'
@isullivan isullivan merged commit 31b6603 into main Apr 5, 2024
2 checks passed
@isullivan isullivan deleted the tickets/DM-43711 branch April 5, 2024 22:37
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

2 participants