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-5304: Use psfex as the default PSF determiner #422

Merged
merged 1 commit into from Nov 3, 2020
Merged

Conversation

parejkoj
Copy link
Contributor

No description provided.

Copy link
Member

@arunkannawadi arunkannawadi left a comment

Choose a reason for hiding this comment

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

I'm not quite sure why so many values need to be hard-coded. Couldn't we compare all the objects with the first object, i.e., i=0 without explicitly listing them?

tests/test_processCcd.py Show resolved Hide resolved
tests/test_processCcd.py Show resolved Hide resolved
@parejkoj
Copy link
Contributor Author

parejkoj commented Nov 2, 2020

I don't want to refactor this test at all: it's a purely empirical test. I'm not a fan of how the test is implemented, but I'd much rather not dig into it here.

@arunkannawadi
Copy link
Member

Sure, I didn't necessarily mean to refactor the test. It'd be helpful to write some instructions as to how you came up with those numbers. If I made an algorithmic change that makes this test fail, I wouldn't know how to find the new set of numbers.

@arunkannawadi
Copy link
Member

Okay, I see the comments now.

@arunkannawadi arunkannawadi self-requested a review November 2, 2020 15:56
Test values changed slightly, but they are purely empirical; add a note
to the test to emphasize that for future developers.
@parejkoj parejkoj merged commit 9cdc311 into master Nov 3, 2020
@timj timj deleted the tickets/DM-5304 branch February 18, 2021 15:49
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