Skip to content

Conversation

kfindeisen
Copy link
Member

@kfindeisen kfindeisen commented Jun 21, 2022

This PR fixes some bit rot in the prototype, adds support for a larger test dataset (up to 4 visits), and removes single-visit-only code from upload.py and the run_pipeline method.

It is not yet possible to handle multiple visits with the prototype, because the Butler management needs to be made more flexible first (DM-35051).

@kfindeisen kfindeisen force-pushed the tickets/DM-35052 branch 4 times, most recently from 1b09d34 to 894d2d7 Compare June 28, 2022 22:34
@kfindeisen kfindeisen marked this pull request as ready for review July 12, 2022 22:07
@kfindeisen kfindeisen requested a review from parejkoj July 12, 2022 22:25
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.

This is a nice improvement. My only real concern is about the "metadata in paths"; a unittest there would help.

Using paths for metadata always makes me nervous, but I guess we have a closed ecosystem here so it should be fine.

"y": "phot_g_mean",
"VR": "phot_g_mean"}
config.astromRefObjLoader.anyFilterMapsToThis = "phot_g_mean"
config.astromRefObjLoader.filterMap = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

These two lines are technically not necessary, since DM-27013 is done.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't know that, but for configuration that depends on the input data, I prefer to be explicit instead of trusting that the defaults match up with I really need.

@kfindeisen
Copy link
Member Author

Using paths for metadata always makes me nervous, but I guess we have a closed ecosystem here so it should be fine.

Note that previously, we were using the filename for metadata, which is even harder to control.

@kfindeisen kfindeisen merged commit 67f5d6a into main Jul 18, 2022
@kfindeisen kfindeisen deleted the tickets/DM-35052 branch July 18, 2022 20:58
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