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-16333: Add Job metadata to the ap job. #54

Merged
merged 3 commits into from Dec 6, 2018
Merged

Conversation

SimonKrughoff
Copy link
Contributor

@kfindeisen and @jhoblitt I think we could do something like this to get the appropriate metadata in the job. Does this look reasonable, and how should I test?

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.

When I first wrote DM-11321 I'd intended for job-level metadata to be the responsibility of the ap.verify.metrics.AutoJob class. However, that entire module is being deprecated in DM-16017, so a temporary hack in pipeline_driver is fine.

However, I'm a bit concerned about how to translate this code into the DM-16017 framework. The camera should be easy enough, but we already have one metric (ap_association.totalUnassociatedDiaObjects) that applies to the entire dataset, and will therefore have an empty data ID. Should it have filter_name = None?

python/lsst/ap/verify/pipeline_driver.py Outdated Show resolved Hide resolved
python/lsst/ap/verify/pipeline_driver.py Outdated Show resolved Hide resolved
@SimonKrughoff
Copy link
Contributor Author

I think in general, we will want to associate the data ids involved in measuring a metric with the metric. That's why I haven't considered including the filter name in the metadata as onerous when it makes sense.

@SimonKrughoff
Copy link
Contributor Author

OK. I'll make the updates and try to test it out. @jhoblitt should we set up a test env to test the branch?

@SimonKrughoff
Copy link
Contributor Author

@kfindeisen Have a look at this updated PR. I've taken into account your comments. Note that we no longer select specific metadata from the dataId, but ship the whole thing assuming it will be useful. Also note that I also had to fix some linting errors. I rebased yesterday, but some of these may still have been fixed. I'll make sure to rebase on master before merging.

python/lsst/ap/verify/pipeline_driver.py Show resolved Hide resolved
python/lsst/ap/verify/pipeline_driver.py Show resolved Hide resolved
python/lsst/ap/verify/pipeline_driver.py Show resolved Hide resolved
python/lsst/ap/verify/pipeline_driver.py Outdated Show resolved Hide resolved
tests/test_dataset.py Outdated Show resolved Hide resolved
@SimonKrughoff SimonKrughoff merged commit c99b4dc into master Dec 6, 2018
@jhoblitt jhoblitt deleted the tickets/DM-16333 branch January 8, 2019 16:21
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