-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add bounds checks to radec_to_desiname #207
Conversation
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.
Thank you for the quick fix, Ben. I am happy to accept your changes as-is but I left two things in-line for you to consider.
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, my previous comments still remain after this update. You're welcome to ignore them if you wish. I have added one additional comment you can choose to address or not.
Feel free to accept or reject any of the three and merge if satisfied.
I will review those soon. I was just very busy debugging the tests yesterday. |
@sbailey, there is a remaining question about how specific we want to make the error messages, for example not just saying "bad value found", but "25 instances of bad values of target_ra found, for example 375 at index 244 of target_ra". It wouldn't necessarily be that hard to implement something like that, but why can't users just do that themselves? |
Based on discussion with @sbailey yesterday, I think this can be merged in the current state. I will merge shortly unless there are further comments. |
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.
Agreed, ok to merge.
This PR closes #206.
In addition the test matrix is updated to bring the tests more in line with Jura (hence the oddball branch name).