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-26553: Create gen3 unittests and test CreateApFakes pipeline tasks on data. #59

Merged
merged 3 commits into from Sep 28, 2020

Conversation

morriscb
Copy link
Contributor

No description provided.

Debug quantum unittest.

Debug unittests.

fix up linting.
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.

Looks good, minor usage comments.

Comment on lines 92 to 93
# Actual input dataset omitted for simplicity
run.assert_called_once()
Copy link
Member

Choose a reason for hiding this comment

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

I skipped this in the example because it would have added a lot of distracting boilerplate, but since you have self.simpleMap and the tract ID, you should be able to predict the inputs to run and call assert_called_once_with.

Comment on lines 94 to 97
# Smoke test that the full pipeline task actualy runs.
testUtils.runTestQuantum(fakesTask, butler, quantum, False)
shutil.rmtree(root, ignore_errors=True)

Copy link
Member

Choose a reason for hiding this comment

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

Seems redundant with testRun, but ok...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I can remove this since my concern here was that the values in runQuantum weren't being extracted properly. The assert_called_once_with resolves this I think.

@kfindeisen
Copy link
Member

Actually, I do have one question that's outside the scope of the review: it looks like CreateRandomApFakesTask generates a fake source catalog given any skymap and tract. Aside from being useful to us, what does it have to do with AP or ap_pipe? Should it go into pipe_tasks instead, perhaps with a more generic name?

@morriscb
Copy link
Contributor Author

There is some specificity here in the sense of the split between calexp and coadd source creation. The idea being they will show up in the diffIm differently. This config can be set up to pull all sources into both or you could just ignore the columns when running the insert tasks. I'm happy to leave this here for now and move it later. If @yalsayyad or @sr525 want, I can create a ticket to move this task into pipe_tasks so it can be used more generally down the road. I did code it as a kind of simple template class for creating fakes so it shouldn't be too hard to move it in the future.

Debug assert_called_with_once
@morriscb morriscb merged commit 8e9662f into master Sep 28, 2020
@kfindeisen kfindeisen deleted the tickets/DM-26553 branch April 13, 2022 21:55
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.

None yet

2 participants