Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
1294 from images #1296
1294 from images #1296
Changes from 15 commits
fd84bce
393c789
a348d0b
7ceb6e8
092c10f
90c24c1
340df3e
fbee5e7
fb6f1fb
7158c7b
4aa0172
9bcc57e
13ccd77
b45e830
14a5032
2bdd01b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If someone uses images with a non-PixelScale wcs, this will give a possibly obtuse error message. Maybe worth adding:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think both of these should be GalSimValueError, not GalSimNotImplementedError.
cf. our description of when to use each error type at the top of errors.py. I think both of these feel more like something that will never work, rather than something that could be implemented down the road.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected.
There was a problem hiding this comment.
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 codecov is reporting the wrong line, but when I run coverage locally, this is the line that isn't covered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, thats odd, I test those errors in lines 1970-1980 of
test_chromatic.py
. Not sure why it says they are not covered.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The trick I use when building up coverage is to add a line before the one I want to check is covered that looks like:
Then if I don't get a
TypeError: exceptions must derive from BaseException
, then I know it never got to that point.Then you can keep adjusting the test until you get that error, at which point remove the added line, and you'll know it's getting to the line you want covered.
If there doesn't seem to be any way to hit this line, then it may be that the condition you are testing is already implicitly tested elsewhere, in which case this test is unnecessary and can be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks that was helpful in finding the issue. It should be solved now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have a doc string, as it is not just an implementation detail, but rather something that users can be expected to be stable API. Just more dangerous than the non-underscore version in terms of the inputs. Our usual docstring would be something like
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we assuming all images have the same pixel scale? If so, that needs to be documented and potentially tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clarification added to documentation and tested in unit tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, our assert_raises is identical to np.testing.assert_raises, so you could have left it as the context version if you prefer that for clarity. But this is fine too.