WIP: Add hypothesis testing for Time precision#9532
Conversation
|
Still need to figure out how to add a testing dependency to astropy. |
On a temporary basis I got it working for #9211; longer term the Decemeber planning meeting will be considering adding Hypothesis as a dependency of |
|
Still need to figure out how to add a testing dependency to astropy.
Yes, we had some discussion on Slack and I basically copied your changes from #9211 (including workarounds for astropy's wonky handling of dependencies in CI). |
|
My suggestion was that we use this, and perhaps also #9211 , as test beds to show that hypothesis can (or can't) serve as an effective tool for ensuring that astropy provides correct and accurate results, and also as a way to evaluate the impact of adding hypothesis on CI loads. |
|
From memory Hypothesis is in the conda-forge channel but not default. |
mhvk
left a comment
There was a problem hiding this comment.
Haven't done a thorough review yet, but I really like where this is going! The tests are all clean examples of what we expect to hold.
My main worry is much more about balancing power usage (I really do care about minimizing that, and after some time, one should be able to know a piece of code works and it is no longer necessary to test all the time), and about possible false positives for unrelated PRs (this is probably less of a deal, since one would see it was an unrelated bug pretty quickly).
For the power usage, just to have some sense: how much does the time required to run all time tests increase due to this PR?
| jd1_b, jd2_b = jds_b | ||
| # not translation invariant so can't convert TimeDelta | ||
| assume(scale1 != 'utc') | ||
| assume(scale1 != 'ut1') |
There was a problem hiding this comment.
UT1 and UT1 should be OK (counted in earth rotation cycles, so continuous). But I guess you really are aiming for different scale1 and scale2 here - in that case, maybe make the test explicitly do that? (skip it if scales are equal?)
There was a problem hiding this comment.
Well it surprises me a little that UT1-UT1 might work so I changed the assumptions to permit that (UTC-UTC does not).
There was a problem hiding this comment.
We had quite a bit of discussion about it at the time, but came to the conclusion that since UT1 is continuous, it should work. It mostly helps in that you can, therefore, just adds quantities in units of time to it (which have indeterminate scale, represented with scale=None in TimeDelta).
|
Any idea why we might be pulling in nose, of all things? One run is dying on a DeprecationWarning internal to nose, but I don't think we use it at all? |
This was my (admittedly related) experience with hypothesis on PINT as well.
The power usage problem is not just restricted to hypothesis tests, of course - in principle one shouldn't need to run the entire test suite on every minor code change. But the usual understanding is that the number of times it catches the fact that new code broke something unexpected is worth it. Certainly hypothesis increases the cost of testing, though how much is an open question (which I will try to address). Note, though, that something like using a test fixture that creates and deletes a temporary directory can also add pretty substantially to the runtime of a test (because it waits for IO operations).
|
|
As should be visible in the diff, hypothesis supports profiles, and (if I got it right) all CI runs use the Certainly I've done some |
|
Yep, 100 runs by default. This is the main knob to tune for test runtime and power consumption - the only impact (assuming that you persist the example database) is that reducing the number of examples gives you a somewhat-more-than-linear decrease in probability of finding a bug in any given run. (mostly because the first few examples let Hypothesis discover the structure of the strategy and start generating valid data, and when you're using |
Oh! I guess we could use the Travis cache on the .hypothesis directory to get (opportunistic) persistence? |
Exactly! Obviously Hypothesis doesn't need persistence, but it does give some nice workflows for caching slow-to-compute values, replaying some passing and failed examples to jump-start generation, and so on. |
What.... Does |
|
Nope, we only depend on |
|
The cache with somewhat fewer runs in default PR testing sounds like a good compromise. And maybe hammer it a bit more before a release or so (or for the cron job?). |
|
The nose mystery -- It only effects one job. So somehow in that particular CI setting, using |
Maybe doesn't even call; I believe |
|
The job that failed, even though we run things in |
Sounds good, I'd probably do both. |
|
So I'm not quite sure what to expect in terms of interaction between Time/TimeDelta and datetime/timedelta. I get differences by integer seconds with UTC, but nontrivial differences with other time scales as well. I can file an issue but I'm not sure what behaviour I should expect. |
|
I'm a little confused by sidereal_time - I thought it should always be between 0 and about 24h*(1-1/365)? It is represented as an Angle, though, so automatic wrapping happens at 24h, which could be trouble, certainly if the user ever does any arithmetic. Maybe "sidereal time" doesn't mean what I think it does? |
|
@Zac-HD - any idea why there is a deadline to fail? both the default profile "normal" and the "ci" profile it is supposed to be using set Perhaps we can ask hypothesis to emit more status information? I haven't seen a deadline failure in a while on my local machine (and they were somewhat regular occurrences until I used the profile to disable them). |
|
Unfortunately I don't know what's happening there - my guess would be that the environment variable isn't being picked up properly? Aside from printing the actual env var value, running either pytest or Hypothesis in verbose mode ( |
|
OK meeting discussion seemed to agree that hypothesis testing can go into astropy. How do we want to handle this?
The main question is how to handle interaction with pytest-astropy; I don't really know how to work on related changes between it and mainline astropy (and the git modules thing just seems like a black box that wrecks things randomly). |
I think making |
I think it would be useful for pytest-astropy to pass through some hypothesis-specific options from "python setup.py test" to the inner "pytest" so one could avoid the "--args" mechanism. |
There doesn't seem to be any documentation on how this quoting should work...
wouldn't docs be nice?
Shells. Ugh.
I think this will be a non-issue when APE 17 is accepted and implemented, as we will no longer be using |
|
I added astropy/pytest-astropy#24 for discussion of |
|
|
|
@aarchiba - I'm remilestoning this to the next bugfix as this may or may not require some more work after the rebase (tests seemed to passed before, so hopefully it won't be too bad). |
|
@aarchiba @mhvk @taldcroft - how do we want to proceed with this? I guess it could be included in a bugfix release after 4.1? @aarchiba - would you be able to rebase this? |
|
@astrofrog - my thinking was also the bugfix release. And also, that it may be worth splitting this up into smaller chunks, with the perfect is the enemy of good motto, having a few tests are better than nothing (note that we already install hypothesis for the CI, so going gradual is totally reasonable). |
|
Hi humans 👋 - this pull request hasn't had any new commits for approximately 5 months. I plan to close this in a month if the pull request doesn't have any new commits by then. In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock. If you believe I commented on this pull request incorrectly, please report this here. |
|
Since There are actually very few merge conflicts in |
Description
This pull request is to ensure that
astropy.timeis thoroughly tested and reliably maintains sub-nanosecond precision. It uses the testing packagehypothesisto identify difficult corner cases.