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-28158: Remove assertion on times that are no longer top far in the future. #279

Merged
merged 1 commit into from Dec 30, 2020

Conversation

TallJimbo
Copy link
Member

No description provided.

Copy link
Contributor

@isullivan isullivan left a comment

Choose a reason for hiding this comment

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

Please fix the typo in the commit message ("top" -> "too"?), otherwise this looks fine.

I'm a little worried this might trip us up again in the future, when the ImSim data might instead be too far in the past. Could you instead catch this specific type of error with a try/except block, or would that defeat the test?

self.assertObservationInfoFromYaml(filename, dir=self.datadir, **expected)
else:
self.assertObservationInfoFromYaml(filename, dir=self.datadir, **expected)
self.assertObservationInfoFromYaml(filename, dir=self.datadir, **expected)
Copy link
Member

Choose a reason for hiding this comment

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

The only comment is that this test was mainly there to hide the warning from the test output so that people don't get confused when they run pytest and see some unexpected warnings turning up. I assume that is not a problem here because now we've hit a time where imsim will never trigger more warnings.

@TallJimbo
Copy link
Member Author

I'm a little worried this might trip us up again in the future, when the ImSim data might instead be too far in the past. Could you instead catch this specific type of error with a try/except block, or would that defeat the test?

I don't think "too far in the past" is a thing; I think the logic astropy is worried about projecting into the future is based on some kind of table for which historical data is not a problem (at least in the modern era).

@TallJimbo TallJimbo merged commit 446a932 into master Dec 30, 2020
@TallJimbo TallJimbo deleted the tickets/DM-28158 branch December 30, 2020 22:24
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

3 participants