Repository navigation
Change default timescale for epochs to 'tt' - #8600
Conversation
|
Today is feature freeze day, so the sooner you can fix the tests, the better. 😅 |
|
So, is this supposed to fully address #8594 or just parts of it? |
| assert tuple(mywcs.wcs.ctype) == ('TLON-CAR', 'TLAT-CAR') | ||
| assert mywcs.wcs.radesys == 'ITRS' | ||
| assert mywcs.wcs.dateobs == Time('J2000').utc.isot | ||
| assert mywcs.wcs.dateobs == Time('J2000').utc.fits |
There was a problem hiding this comment.
Looking at this again, I realize the change is actually not necessary, but it is probably good to mention that dateobs will now be '2000-01-01T11:58:55.816' - it would probably be better if the time system was also stored (as it is in FITS).
| from astropy.utils.exceptions import AstropyWarning | ||
|
|
||
|
|
||
| # The UTC time scale is not properly defined prior to 1960, so Time('B1950', |
There was a problem hiding this comment.
Removed the comment since it is now basically consistent, though note that I just went for tt for B1950 too.
Codecov Report
@@ Coverage Diff @@
## master #8600 +/- ##
==========================================
+ Coverage 86.9% 86.91% +<.01%
==========================================
Files 393 393
Lines 58584 58587 +3
Branches 1064 1064
==========================================
+ Hits 50914 50919 +5
+ Misses 7051 7049 -2
Partials 619 619
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #8600 +/- ##
==========================================
+ Coverage 86.95% 86.95% +<.01%
==========================================
Files 399 399
Lines 59329 59332 +3
Branches 1100 1100
==========================================
+ Hits 51587 51590 +3
Misses 7101 7101
Partials 641 641
Continue to review full report at Codecov.
|
|
(I do prefer your implementation on the time side here, though, over mine) |
|
Agreed that at least in the actual code, it is clearer to explicitly set the time-scale. I do think that for the tests it is better not to set the scale explicitly to something we now feel is wrong, and I took the lazy way out... More on topic: I would certainly prefer to have @taldcroft sign off on this, though in principle I guess it could be to revert if he feels this is a bad idea. Practically, I've run out of time here... Since this is technically a bug (though one with repercussions), perhaps we can postpone it? |
taldcroft
left a comment
taldcroft
left a comment
There was a problem hiding this comment.
.. back from vacation.
This looks good to me. I'm generally in favor of making the changes in time and coordinates in one go for this release. I just wonder about getting appropriate visibility. Maybe a What's New mention?
It's kind of a bigger topic, but I do find it useful (as done in e.g. numpy) to have the version when things changed or were added in the narrative / API docs. E.g. in relevant places we would see a comment like "Default time scale for epoch times like J2000 changed from UTC to TT in version 3.2". The point being that the change log is not where most people wind up when learning about a function or method.
|
👍 to (an equivalent of) |
|
To clarify one thing since there's some cross-conversation... I did this part a few days ago:
in #8594. I think that's good to merge if @mhvk gives it a 👍 (and then this can be rebased on that) |
|
Please don't wait for me - I'm off traveling. So, maybe someone else can do the review? With everybody agreeing that we should propagate this to |
c69b907 to
ae0e35a
Compare
|
I rebased this just in case we want to merge just this PR. I'm about to board, but think I'll have some time later in the week to rebase if the other PR is merged first. |
|
ps. Do feel free to just edit & merge without my comments; I think we all agree what needs to be done here! |
eteq
added
the
zzz 💤 merge-when-ci-passes
Change default timescale for epochs to 'tt'
See discussion in #8594.
Still to do: adjust coordinate tests to not set time scale for equinoxes to 'utc'