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-32615: Move jointcal cfht_minimal tests to gen3 #207

Merged
merged 10 commits into from Dec 13, 2021
Merged

Conversation

parejkoj
Copy link
Collaborator

No description provided.

Copied from testdata_jointcal/cfht, so that all the relevant fields are
updated for the postprocess script.
Copy link
Contributor

@taranu taranu left a comment

Choose a reason for hiding this comment

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

I mostly had questions, and one more serious typo to fix.

It would be helpful to note somewhere (on the ticket if not in the README) how you generated the exports.yaml.

python/lsst/jointcal/testUtils.py Outdated Show resolved Hide resolved
tests/data/cfht_minimal_sourceTable.yaml Outdated Show resolved Hide resolved
tests/data/make_butler_repo.py Outdated Show resolved Hide resolved
from lsst.daf.butler.script import ingest_files
import lsst.obs.base

repopath = "cfht_minimal/repo"
Copy link
Contributor

Choose a reason for hiding this comment

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

os.path.join(os.environ["JOINTCAL_DIR"], 'tests/data/cfht_minimal/repo') doesn't work? (If not here, then in the butler.import_ call?) But also, I don't see absolute paths in sdss-dr9-fink-v5b.ecsv.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I couldn't figure it out: the paths in the ecsv file are all relative, so they should have been relative to whatever prefix= I passed in to ingest_files. I believe that I want the repopath to be relative too, so that everything inside the butler is relative to that (though I can't confirm that changing it the way you suggest would cause butler-internal paths to be absolute).

tests/data/postprocess_cfht_minimal.py Outdated Show resolved Hide resolved
with self.assertRaisesRegex(RuntimeError, "No data to process"):
jointcal.JointcalTask.parseAndRun(args=args, doReturnResults=True, config=self.config)

def test_jointcalTask_fails_no_raise(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it safe to remove this test completely?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes: this is testing gen2-only functionality here. The test_jointcalTask_fails_raise test demonstrates that JointcalTask will raise if there is no data, but whether it raises in a pipetask context, or just goes on to the next quantum and logs a failure is entirely up to the pipetask configuration and not something I can test within jointcal itself.

instrInstance.register(butler.registry)

graph = butler.registry.dimensions.extract(["htm7"])
datasetType = lsst.daf.butler.DatasetType("sdss_dr9_fink_v5b", graph,
Copy link
Contributor

Choose a reason for hiding this comment

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

Seeing how often sdss_dr9_fink_v5b appears everywhere, would it make sense to define a refcat_default = "sdss_dr9_fink_v5b" somewhere in the package?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the package, or just in this file? The sdss refcats are really only used in a couple of tests, so they definitely shouldn't be package-wide defaults. Those are Gaia DR2 for astrometry and PS1 for photometry.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, fair enough. Maybe we should have some refcat names defined in a package somewhere because it seems like it'll be a huge pain if we ever want to update them en masse.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Eh, these particular refcats will probably never get updated, since they are each for a specific testing purpose. If we add e.g. a catalog with proper motion covariances or something, I'll just add a specific test for that catalog.

For non-jointcal packages, I think we'll be consolidating some of those names as part of the drp_pipe work, and I plan to tackle DM-27013 over the break, which should help.

Gen3 jointcal needs the parquet tables, and we need to tweak how they are made
because this is much older data that is missing many expected fields.
This file is made from the testdata_jointcal/cfht/exports.yaml, modified for
the layout of the gen2 files in cfht_minimal.
These files were generated with the postprocess_cfht_minimal.py script.
Remove no_raise tests, as that is all handled internal to pipetask now;
keep the test of raising when there is no data since that is the
correct behavior now, always.

Add minSnr config for cfht_minimal tests.
Modify cfht_minimal-config to reflect the fact that the catalogs come from
much older processing.
Add sdss refcat .ecsv ingest file

Update test data readme
tests/data/cfht_minimal now has a working stripped down gen3 repo to use for
these tests.

Cleanup some "ccd"->"detector" names

Remove unnecessary gen2 butler import from test_photometryModel.
@parejkoj parejkoj merged commit 6e02a3c into main Dec 13, 2021
@parejkoj parejkoj deleted the tickets/DM-32615 branch December 13, 2021 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants