Repository navigation
Fix jd1 and jd2 format - #6653
Conversation
|
Hi there @Juanlu001 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃. Everything looks good from my point of view! 👍 If there are any issues with this message, please report them here. |
|
@Juanlu001 - the first error is not a problem - in that case, it is fine to adjust the test, but the second is weird... |
|
@Juanlu001 - that first test failing is OK, since the jd1 and jd2 are now different. The second one is a bit weirder, but is the result of the fact that with But it turns out there are more tests that fail... I fixed those, plus the failing doctests, and am left wondering if this is worth it. The alternative would be to adjust |
|
I guess this deserves deeper discussion then. I would love to hear other opinions before going forward as well. |
|
I was hit by this today again, and I would like to try the alternative approach of adjusting |
The time arithmetic is again causing a loss of precision, see astropy/astropy#6653 for an attempt to fix it.
|
@Juanlu001 - with a bit more thought, I think I've come around to liking this approach more - we had those |
|
👍 on this approach. I'm not sure it is perfect [1], but certainly a real improvement and consistent with practice elsewhere. Of course it is failing the new tests at the moment... @Juanlu001 - was it passing tests locally? That should normally be a precondition before pushing to github. [1]: But I do see the point that this solution is probably as good as necessary since the Julian Date format by definition is starting from float64 precision. |
5a15e75 to
3db5ab3
Compare
|
@taldcroft Yes, it's failing because I reverted - but I just reverted the reversion :) @mhvk There were some failing doctests on Windows and OS X in your last commit, though. |
|
Closing and reopening. |
|
I cannot reproduce these failures locally. Trying to match the output of the CI yields this: Details |
|
I tried several NumPy versions, default and conda-forge, with and without MKL... still can't reproduce. Not sure what's happening here 😕 |
|
@Juanlu001 - the failing doctest should be fine to just fix. This looks like an acceptable change in the jd1/2 representation, from I'm a little confused about saying "I cannot reproduce these failures locally". I am able to reproduce the current CI failures, and it looks like you did too. ?? |
|
You were right, it worked! Probably an installation problem on my end. Should I add a changelog entry or something? |
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
I think this looks all OK. I would add a changelog entry since I feel this has fixed a small buglet. That would mean it should also be backported, so add it under 2.0.3. It can just refer to the fact the initialization was made consistent for all formats.
76b4f5b to
df075b8
Compare
|
Done! |
This is an attempt to fix #6638. However, as it can be seen in the last commit I didn't fix the original issue: this is because having the same order of magnitude is not enough to achieve perfect precision.
On the other hand, as advised by @mhvk I tried to do
self.jd1, self.jd2 = day_frac(jd1, jd2)forTimeStringas well, but several things broke. With this diff:I got several test failures, and the thing is that I am not sure what do do next.
Details