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-27147: Revert to older skymap dataset type names in processing. #114

Merged
merged 1 commit into from Nov 26, 2020

Conversation

TallJimbo
Copy link
Member

No description provided.

# (and collection naming convention) and this config override can be
# removed (other changes will probably be necessary as well, to reflect
# changes to the collection naming conventions).
pipelineArgs.extend(["--config", "imageDifference:connections.skyMap='{skyMapName}Coadd_skyMap'"])
Copy link
Member

Choose a reason for hiding this comment

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

I think this will work, but since it doesn't depend on anything run-time, it might be cleaner to put it in pipelines/ApVerify.yaml.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. I'll test this first since it's in hand, then switch to that and make sure it still works.

Copy link
Member Author

Choose a reason for hiding this comment

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

So, I think that if I were to move the config override to the pipeline file, I'd have to duplicate the config overrides in the ap_pipe pipeline file, because a task: entry in an inherited pipeline replaces the inherited one instead of being merged with it (@natelust, can you confirm that?). That makes me lean towards leaving this here, but I'm happy to follow your lead if you happen to look again tonight.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the class pointed to by the label is the same in the base pipeline and the derived pipeline they are merged not replaced.

Copy link
Member Author

Choose a reason for hiding this comment

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

Excellent, I'll go with to the pipeline solution, then. Thanks!

Comment on lines 172 to 177
# TODO: this config option resets the skymap dataset type to the templated
# name used by default prior to DM-27147. After RFC-471 is adopted, the
# ap_verify test data should be migrated to use the new dataset type name
# (and collection naming convention) and this config override can be
# removed (other changes will probably be necessary as well, to reflect
# changes to the collection naming conventions).
Copy link
Member

Choose a reason for hiding this comment

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

I suggest simplifying this to "TODO: remove after ap_verify datasets follow RFC-471 RFC-741 conventions".

@TallJimbo TallJimbo merged commit b0e9dc4 into master Nov 26, 2020
@TallJimbo TallJimbo deleted the tickets/DM-27147 branch November 26, 2020 05:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants