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-19470: Create jointcal PipelineTask (version 0: tests) #165

Merged
merged 6 commits into from Dec 14, 2020

Conversation

parejkoj
Copy link
Collaborator

@parejkoj parejkoj commented Dec 8, 2020

No description provided.

@parejkoj parejkoj force-pushed the tickets/DM-19470 branch 2 times, most recently from b331d96 to 8afdd55 Compare December 9, 2020 00:16
Copy link
Contributor

@erykoff erykoff left a comment

Choose a reason for hiding this comment

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

Not a huge number of comments, but I want to make sure this is a solid foundation for upcoming changes.

class JointcalTask(pipeBase.CmdLineTask):
"""Jointly astrometrically and photometrically calibrate a group of images."""
@dataclasses.dataclass
class JointcalInputData:
Copy link
Contributor

Choose a reason for hiding this comment

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

I see where you're going here with making the common dataclass for gen2 and gen3 to normalize things. However, I'm not sure if this will actually work (which is maybe part of the point of the new ticket?) because (a) the gen3 version (at least!) should be using the parquet files; (b) by reading all the catalogs first and not cutting them down as you read them this leads to significant memory overhead. But having a common dataclass in general does make sense.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

a) We'll see how this looks when I write the Parquet version. This class may not be as helpful then.

b) The gen2 code is still only reading one catalog/image at a time, so no memory pressure there. The dataclass is just to help separate the "read input" code (which is different in gen2/gen3) from the "create internal jointcal representation" code (which is middleware agnostic).

tests/jointcalTestBase.py Outdated Show resolved Hide resolved
tests/jointcalTestBase.py Show resolved Hide resolved
tests/jointcalTestBase.py Outdated Show resolved Hide resolved
tests/test_jointcal_cfht_minimal.py Show resolved Hide resolved
@parejkoj
Copy link
Collaborator Author

While looking through your comments, I noticed that I'd not written docstrings for the new test methods. I've added those and some cleanups on one commit. @erykoff : mind taking a quick look at the new commit? You might want to borrow some of it for your own fgcmcal test helpers.

Copy link
Contributor

@erykoff erykoff left a comment

Choose a reason for hiding this comment

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

See comment.

transfer='symlink',
skip_dimensions={'instrument', 'detector', 'physical_filter'})

def _runPipeline(self, repo, queryString=None,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why you don't want to use pipeline files. It's how the system is supposed to be run (though I suppose in a test it can be simulated, that's swimming upstream).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I'll add it back in on DM-27869, where I can actually try it out.

@parejkoj parejkoj merged commit 642e80a into master Dec 14, 2020
@parejkoj parejkoj deleted the tickets/DM-19470 branch December 14, 2020 22:38
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