Parse and do float comparison of float output in doctests#2087
Conversation
|
If this can be made to work then we could even remove a lot of the ellipses from the existing doctests. |
|
👍 to the idea! |
|
I have a simple prototype of this already working, but I don't want to go too much further with it until we have something like #2050 in place, since it'll be a pain to test otherwise. I'm still not sure how to square my current implementation with the ELLIPSIS flag either. Currently if a test is using my FLOAT_COMPARISON flag it forces ELLIPSIS to be disabled. I might need to roll my own parsing of ellipses into the solution to make it complete. |
|
Attached is aforementioned prototype. |
There was a problem hiding this comment.
It looks like self.normalize_floats can return None, so maybe we need to handle that case here?
There was a problem hiding this comment.
Oops, that's a bug. It should probably just return want and got unmodified.
|
This is really cool. Gives me some motivation to work on #2050... |
|
Would be wonderful! |
|
This is the pendant in sympy sympy/sympy#2051 |
|
@embray - it looks like the Travis failure is related? |
|
Thanks @Thisch, I'll have to look into it. Maybe I can just "borrow" your work on this from SymPy :) |
|
@embray go ahead! |
|
I've recently run into issues with floats in doctests in an affiliated package. |
|
@embray - shall we re-schedule to 1.0? |
|
This would be really nice to have, for affiliated packages as well. I'll try to see if I can get this working tomorrow using the code from sympy. |
|
@embray @astrofrog - perhaps we can re-schedule this to 0.4.1 instead of 1.0? If I understand right it's actually a doc fix, so it's permissible in a bugfix version? |
|
👍 |
|
We'll see. It's been high on my todo list. Let me see if I can get to it. |
|
Rebase and, as you can see, added the code from SymPy to do the float comparison. The original test I wrote for my own code passed with the SymPy code too, which is a good sign. I'm going to start looking at what doctests I can get passing with this enhancement. |
|
I updated several of the existing doctests to make use of the If anyone has time to kill they can try rewriting more of the doctests to use this feature, but otherwise it's maybe something to update as we go along. One potential downside to this is that it means the doctests will be more sensitive to small changes in numeric results. Those changes could be significant and/or unintended in some cases, but unintentional in other cases. I think it's still better to err on the side of caution and, if some algorithmic changes result in slight differences then the author can check whether the difference is important and update the tests accordingly. |
…nce astropy#2087 is implemented the ellipses can be removed.
|
Woot--build passed. Any objections to merging this for 0.4? One last thing I might do is add some documentation for this feature. |
…nce astropy#2087 is implemented the ellipses can be removed.
|
Great! No objections from me :) |
|
@embray - the last few coordinates PRs have added a lot of places in the coordinates documentation. where But of course it also works fine as-is right now, so either way works for me. And I'm 👍 on merging this regardless! |
|
@eteq I already fixed a bunch of these in coordinates, but maybe I'll rebase and do a few more. Using this in more tests has helped iron out a few small corner cases so far. |
|
Wonderful, all in favour of merging! Also note that there is one more equivalency doctest that came in recently ( |
…do about ellipses. Also note the warning that this could hide subtle differences in floating point representation that *could* be a bug, but also points out that there probably ought to be separate unit tests for issues like that.
Added code from Sympy's output checker for handling floating point values in output. This is similar to my previous approach but takes it further, resolving most of the shortcomings of my original approach. I merged our existing OutputChecker subclass with the code from SymPy into a new AstropyOutputChecker. As roughly 100 lines of code were borrowed from the SymPy project I included a copy of their license.
…eature, renamed FLOAT_COMPARISON to just FLOAT_CMP for the terseness.
…racters that may indicate the 'beginning' of a float. I think this is a bit arbitrary in fragile, as evidenced by the fact that I had to add a left-parens to the list, as it can certainly precede a float. Might change this later, but it's okay for now I guess.
…ppears, found through usage.
…ercise the feature very fully
|
Rebased. Updated several more of the coordinates test cases, and added documentation. After the next build passes I'll merge this for 0.4. I won't be updating any more existing doctests in this PR or it'll never get done :) But once this is in people are free to contribute to that process. |
Parse and do float comparison of float output in doctests
Parse and do float comparison of float output in doctests
This is a suggestion I've made in a few places but I don't think I ever really made a ticket for it, so here it is: Many issues with doctests arise when slight changes in numerical code between Astropy versions, Python versions, or worst of all platforms result in slight differences in floating point results, causing the doctests to fail, since they work entirely on string comparison. This is even worse because on different platforms the default string formatting for different floating point values can differ.
Most of this time we address this using ellipses (
...) to ignore values above a certain decimal place, a feature supported by doctest. But this does not even work for all cases--for example I recently had a case where the doctest expected something like1.000000000000001e-23, but my system was rounding this to1e-23. I suppose this could be written1...e-23in the doctest, but that's pretty unclear. (ETA: I also found that on a different Python version the same test returned something like9.999999999999e-24which is pretty much impossible to deal with using the ellipses approach)What if instead the doctest runner could actually parse floating point strings into actual float values and do a direct numerical comparison? Optionally, the comparison could even be made with a specific tolerance.
This could be cumbersome on tests that don't expect floating point values in the output, so I would suggest this be doing with a new doctest directive, perhaps
# doctest: +FLOAT. This would use a regular expression to parse out all floating point values found in expected and actual test output, in order, so that they can be matched up one for one and compared. This flexible approach would allow it to work with floating point values that appear anywhere in the output, such as in thereprfor aQuantity.