Add Local Earth Rotation Angle. Adjust sidereal time for the TIO. by mkbrewer · Pull Request #11680 · astropy/astropy

mkbrewer · 2021-05-01T14:52:29Z

Description

This pull request is to address my request in issue #10239 for the Local Earth Rotation Angle (eral). It also corrects the local sidereal time for the TIO in models that include it. My preference would be to also change the meaning of greenwich to be the angle at the TIO (i.e. GMST, GAST, or ERA), but I don't think @mhvk would go along with that.

Fixes #10239

pep8speaks · 2021-05-01T14:52:32Z

Hello @mkbrewer 👋! It looks like you've made some changes in your pull request, so I've checked the code again for style.

There are no PEP8 style issues with this pull request - thanks! 🎉

Comment last updated at 2021-05-22 22:38:01 UTC

mkbrewer · 2021-05-01T15:50:58Z

The sidereal time checks will likely fail due to the addition of the TIO, which amounts to around 500 ns. In fact, this is much less than the ~30 us uncertainty in DUT1 though. I've also not added any checks for the Local Earth Rotation Angle. I am unable to do this as I can't actually run Astropy on my computer, so I'll leave this to others.

mkbrewer · 2021-05-01T15:55:42Z

I have no idea what this is all about:

/home/docs/checkouts/readthedocs.org/user_builds/astropy/envs/11680/lib/python3.8/site-packages/astropy/time/core.py:docstring of astropy.time.core.Time.to_local:9: WARNING: py:obj reference target not found: Earth
/home/docs/checkouts/readthedocs.org/user_builds/astropy/envs/11680/lib/python3.8/site-packages/astropy/time/core.py:docstring of astropy.time.core.Time.to_local:9: WARNING: py:obj reference target not found: rotation
.
.
.

mkbrewer · 2021-05-01T16:42:48Z

Yay! I figured it out.

mhvk

@mkbrewer - thanks so much for the PR! I had a quick look and see some parts that should fail - did you try it out yourself? Or am I wrong?

Regardless, we will need some tests that check the code works as expected (in astropy/time/tests/test_sidereal.py and perhaps also astropy/time/tests/test_precision.py). Also, we'd need to describe the new method in the documentation, probably easiest in the section about sidereal time, https://docs.astropy.org/en/latest/time/index.html#sidereal-time (which corresponds to docs/time/index.rst). I'm happy to help with both.

I note in-line that a lot of the code duplicates what we just added to erfa_astrom.apio - ideally, we just have it in one place. Though I think it is best to first just get everything working properly.

astropy/time/core.py

mhvk · 2021-05-08T02:03:54Z

@mkbrewer - I've gone ahead and made sure the code ran and added some tests and documentation.

Looking at the code again, I also realized sidereal_time could trivially do the required calculation as long as I added a new entry in SIDEREAL_TIME_MODELS - I think that is simpler, and has the advantage that if someone looks at the docstring of sidereal_time, they will also be pointed to the existence of earth_rotation_angle (I did keep that method, since I think it is well worth having a simple shortcut for which one has to put in no extra arguments except a possible longitude).

But of course, this means you now better review my contributions!

mhvk · 2021-05-08T02:08:45Z

On your comment on top:

My preference would be to also change the meaning of greenwich to be the angle at the TIO (i.e. GMST, GAST, or ERA)

I agree we should provide a way to just get the angle out without the _to_local method. I think re-using 'greenwich' for that is a bit strange, but might 'tio' make sense? Or perhaps False?

p.s. This reminds me that nominally a string should be an observatory, and we now have EarthLocation.of_site, so really that should be done too...

mkbrewer · 2021-05-08T15:06:27Z

@mhvk I'm afraid that I must disagree with this re-implementation. Just because something can be done, that doesn't mean that it should be done. Sidereal time and Earth rotation angle are different beasties. Allowing the return of Earth rotation angle through the call of a public method called sidereal_time just isn't right.

Furthermore, while sidereal time is a general term which can be taken to mean either Greenwich sidereal time or local sidereal time, Earth rotation angle specifically refers to the angle between the CIO and the TIO. As currently written, the method now called earth_rotation_angle always returns the local Earth rotation angle and never the Earth rotation angle itself. As you say, this could be mitigated by accepting the argument 'tio' (in place of 'greenwich') to return the actual Earth rotation angle rather than the local Earth rotation angle.

My argument for interpreting the string 'greenwich' to mean Greenwich sidereal time in the sidereal_time method is that it is very likely that users are expecting Greenwich sidereal time when they use this argument already. In fact, 0 longitude isn't even the longitude of the Royal Observatory Greenwich, which is given as -0.001475 in sites.json for the EarthLocation 'greenwich'.

It might be a good idea to accept an EarthLocation object in the longitude arguments for both sidereal_time and earth_rotation_angle in addition to a longitude, None or the strings 'greenwich' in the case of sidereal_time or 'tio' in the case of earth_rotation_angle. That way, if someone actually wants the local sidereal time or local Earth rotation angle at Greenwich, they can just use the argument EarthLocation.of_site('greenwich'). This would also be consistent with usage in the solar system method get_body, which uses this approach.

docs/time/index.rst

astropy/time/core.py

mhvk · 2021-05-08T15:17:09Z

OK, that's fair. I'll revert allowing 'era' in sidereal_time proper but will factor out the code in common.

mkbrewer · 2021-05-08T16:07:12Z

Oh, one more thing. There is apparently an exception that can occur in getattr(time, scale) that is handled in get_jd12(). If that is the case, this should also be handled in _erfa_sidereal_time().

mhvk · 2021-05-08T17:10:51Z

Just to be sure that I understand your suggestion: is it that 'greenwich' will mean just return the output of era00, gmst.., etc., i.e., not to apply the TIO locator at all, or just not to apply the polar motion correction?

The former I think is fine, since it means that there is no change from before; the latter probably is OK too.

I also like your idea of just allowing one to pass in an EarthLocation - in the method there really isn't that much sense to allow arbitrary strings for observatories, since normally the Time should just be initialized with the correct location.

mhvk · 2021-05-08T17:19:53Z

On the exception catching in get_jd12 - this is specifically for a time out of range of the IERS table. For sidereal time, so far my tendency has been that the user should take some explicit action if they are happy with times that are off by up to 1s. My tendency would be to do the same for ERA (at least let's do that for this PR; in my revised version, all erfa functions are called the same way).

mkbrewer · 2021-05-08T17:40:15Z

Just to be sure that I understand your suggestion: is it that 'greenwich' will mean just return the output of era00, gmst.., etc., i.e., not to apply the TIO locator at all

Correct. These are mathematical quantities that are useful for use in other equations by users.

mkbrewer · 2021-05-08T17:43:19Z

I guess what you're saying is that you'd prefer that the exception not be handled. If so, that's fine.

mhvk · 2021-05-08T18:02:58Z

OK, that means that for 'greenwich', the answer will not change from what it was before. That seems good!

mkbrewer · 2021-05-08T18:19:27Z

Seems good to me also. Doing otherwise would break code that expects Greenwich sidereal time.

mhvk · 2021-05-08T18:25:14Z

OK, I now also allow EarthLocation input, and 'greenwich' and 'tio' mean ignore the locator. I only mention 'tio' for earth_rotation_angle - here, of course we have real freedom, so let me know if you think this is not a useful marker.

mhvk · 2021-05-08T18:54:44Z

Failure seems unrelated - see #11717

astropy/time/core.py

mkbrewer · 2021-05-08T23:06:17Z

OK, I now also allow EarthLocation input, and 'greenwich' and 'tio' mean ignore the locator. I only mention 'tio' for earth_rotation_angle - here, of course we have real freedom, so let me know if you think this is not a useful marker.

Oh, I forgot to address this. I think it is fine, desirable in fact, to allow both 'greenwich' and 'tio' for Greenwich sidereal time. That way, those who like consistency can use 'tio' for both Greenwich sidereal time and Earth rotation angle, while the change won't break anything for users who are using 'greenwich' currently.

astropy/time/core.py

…culation.

… TIO.

…s text.

mkbrewer · 2021-05-22T16:16:36Z

@mhvk @taldcroft In this comment: #11680 (comment) I was asked to catch all strings other than tio and greenwich. I did that, but it seems that a string is also valid in Longitude, which caused this to fail:

1162   >>> t.sidereal_time('apparent', '-90d')  # doctest: +FLOAT_CMP +REMOTE_DATA
UNEXPECTED EXCEPTION: ValueError('Invalid longitude string')
Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.7.10/x64/lib/python3.7/doctest.py", line 1337, in __run
    compileflags, 1), test.globs)
  File "<doctest index.rst[148]>", line 1, in <module>
  File "/home/runner/work/astropy/astropy/.tox/py37-test-devdeps/lib/python3.7/site-packages/astropy/time/core.py", line 1904, in sidereal_time
    raise ValueError("Invalid longitude string")
ValueError: Invalid longitude string

How should this be resolved?

mhvk · 2021-05-22T16:22:54Z

@mkbrewer - as you see, I just reverted that change. @taldcroft's request was logical but clearly neither he nor I realized at that moment that strings were actually generally allowed as input!

mkbrewer · 2021-05-22T16:51:23Z

Thanks.

mkbrewer · 2021-05-24T16:50:18Z

@taldcroft Might you be able to give me an estimate of when you will have the time to take a look at this? If so, I'll look back then rather than checking hourly to see if there has been an update.

mkbrewer · 2021-06-02T15:16:45Z

@mhvk @taldcroft Has this PR somehow fallen by the wayside? I addressed all of the comments and I'd like to be done with this.

mhvk · 2021-06-02T17:48:05Z

@taldcroft - would you have time to look at this Earth Rotation Angle PR again? I think your (good) comments were all addressed, but would rather not dismiss your "request for changes" without you having another look.

mkbrewer · 2021-06-19T14:35:34Z

@mhvk It's now been a month since @taldcroft 's initial review and still no followup. How long must we wait?

mhvk · 2021-06-19T16:48:24Z

@mkbrewer - let's get it in by the end of the week - I do think it is all OK. (Of course it doesn't really matter in the larger scheme, as it will only get in 5.0, which will be the Fall, but I do similarly like to have things properly off my plate!).

taldcroft

LGTM.

mhvk · 2021-06-20T12:01:21Z

OK, let's get it in! Thanks, @mkbrewer, that was a nice collaboration!

maxnoe · 2021-06-20T12:19:10Z

astropy/time/core.py

+            string  'greenwich' or 'tio', the result will be relative to longitude
+            0 for models before 2000, and relative to the Terrestrial Intermediate
+            Origin (TIO) for later ones (i.e., the output of the relevant ERFA
+            function that calculates greenwich sidereal time).


I think this should mention that if a plain number is passed, it is assumed to be degrees, which I think is a bit surprising, as plain numbers are usually interpreted as radians, right?

It is really for backward compatibility for sidereal_time - if I had written this now I would have insisted on a unit (and plain Longitude input does). So, my sense is not to advertise it.

Wouldn't we allow to change this as this targets 5.0? It seems like a good change to me to require a unit

Not quite sure whether it is worth it, but I guess we could indeed have a DeprecationWarning starting in 5.0.

I'm a bit confused by this. The documentation used to say:

The longitude on the Earth at which to compute the sidereal time. Can be given as a `~astropy.units.Quantity` with angular units (or an `~astropy.coordinates.Angle` or `~astropy.coordinates.Longitude`)

In fact, as we found out, one can also use a string (e.g. '10.2345d'). I'm not sure why this was dropped, but the code hasn't changed in that respect. Isn't the Longitude constructor intelligent enough to convert all of these to degrees if given another angular unit?

Oh, I think I understand. Although this is not mentioned, one can also use a float as the argument, in which case it is assumed to be degrees. I agree with @maxnoe on this. We should have mentioned that in the documentation. Too bad he didn't bring this up a bit earlier. In some respect, I don't think it matters much though. People don't usually specify the longitude of an observatory in radians anyway. And, if they did, they would soon find out that the result would be incorrect.

Yes, I guess it should have been "or anything that can initialize a Longitude" - the fact that float is also allowed is extra (because we give a default unit of u.deg in the initializer). The question is whether it is worth doing a quick follow-up PR that deprecates that behaviour (and perhaps clarifies the docstring).

I'll leave that entirely up to you. Obviously, sidereal_time() has been around for awhile, so any confusion will only affect new users and people who have been using it for years may not appreciate depreciation warnings, but if you feel strongly that a unit should be specified, then go ahead and require one.

@mkbrewer I didn't habe a look at this earlier because I only noticed this PR because the corresponding issue Was closed by merging it ;)

And I am very happy to have it closed after being open for over a year!

github-actions bot added the time label May 1, 2021

pllim added this to the v5.0 milestone May 1, 2021

pllim added the whatsnew-needed label May 1, 2021

nstarman mentioned this pull request May 1, 2021

Developer Documentation for physical types and intersphinxing #11675

Open

mhvk reviewed May 7, 2021

View reviewed changes

mhvk force-pushed the issue-10239-eral branch from 717c61b to ec68e6f Compare May 8, 2021 02:00

mkbrewer commented May 8, 2021

View reviewed changes

docs/time/index.rst Outdated Show resolved Hide resolved

mkbrewer commented May 8, 2021

View reviewed changes

astropy/time/core.py Outdated Show resolved Hide resolved

mhvk force-pushed the issue-10239-eral branch from 635188b to 05b9203 Compare May 8, 2021 18:23

mhvk mentioned this pull request May 8, 2021

Adjust test for matplotlib version failure #11717

Merged

mkbrewer commented May 8, 2021

View reviewed changes

astropy/time/core.py Outdated Show resolved Hide resolved

mkbrewer commented May 8, 2021

View reviewed changes

astropy/time/core.py Outdated Show resolved Hide resolved

mhvk reviewed May 22, 2021

View reviewed changes

astropy/time/core.py Outdated Show resolved Hide resolved

mkbrewer and others added 11 commits May 22, 2021 12:08

Add local earth rotation angle. Adjust sidereal time for the TIO.

50053d1

Fix PEP8 issues. Add changelog entry.

5b1b07c

Attempt to correct docstring format. Convert longitude to radians value.

22e5b4c

Correct use of units

cfb78cd

Add very basic tests of ERA

f845454

Add basic documentation of Earth rotation angle

c8bace3

Slightly reword changelog entry

905dd81

Factor our common parts in sidereal time and earth rotation angle cal…

de30a97

…culation.

Allow location as input for longitude; let 'greenwich' be relative to…

9cf9c58

… TIO.

Fix incorrect TIO/polar motion correction, and equation of the origin…

a96ffd0

…s text.

Implement requested changes.

1463e16

Add test for string longitude input such as '120d'

17e72e2

mhvk force-pushed the issue-10239-eral branch from e00a150 to 17e72e2 Compare May 22, 2021 16:20

mkbrewer added 2 commits May 22, 2021 13:21

Missing commas.

3bcd56a

Add changelog entry noting API change for sidereal time.

e37b99d

taldcroft approved these changes Jun 20, 2021

View reviewed changes

mhvk merged commit f7282a1 into astropy:main Jun 20, 2021

maxnoe reviewed Jun 20, 2021

View reviewed changes

mkbrewer deleted the issue-10239-eral branch June 20, 2021 21:02

mhvk mentioned this pull request Jun 21, 2021

Fix wrong docstring section headers [skip ci] #11867

Merged

Uh oh!

Conversation

mkbrewer commented May 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

pep8speaks commented May 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2021-05-22 22:38:01 UTC

Uh oh!

mkbrewer commented May 1, 2021

Uh oh!

mkbrewer commented May 1, 2021

Uh oh!

mkbrewer commented May 1, 2021

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mhvk commented May 8, 2021

Uh oh!

mhvk commented May 8, 2021

Uh oh!

mkbrewer commented May 8, 2021

Uh oh!

Uh oh!

Uh oh!

mhvk commented May 8, 2021

Uh oh!

mkbrewer commented May 8, 2021

Uh oh!

mhvk commented May 8, 2021

Uh oh!

mhvk commented May 8, 2021

Uh oh!

mkbrewer commented May 8, 2021

Uh oh!

mkbrewer commented May 8, 2021

Uh oh!

mhvk commented May 8, 2021

Uh oh!

mkbrewer commented May 8, 2021

Uh oh!

mhvk commented May 8, 2021

Uh oh!

mhvk commented May 8, 2021

Uh oh!

Uh oh!

Uh oh!

mkbrewer commented May 8, 2021

Uh oh!

Uh oh!

mkbrewer commented May 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhvk commented May 22, 2021

Uh oh!

mkbrewer commented May 22, 2021

Uh oh!

mkbrewer commented May 24, 2021

Uh oh!

mkbrewer commented Jun 2, 2021

Uh oh!

mhvk commented Jun 2, 2021

Uh oh!

mkbrewer commented Jun 19, 2021

Uh oh!

mhvk commented Jun 19, 2021

Uh oh!

taldcroft left a comment

mkbrewer commented May 1, 2021 •

edited

Loading

pep8speaks commented May 1, 2021 •

edited

Loading

mkbrewer commented May 22, 2021 •

edited

Loading

maxnoe Jun 20, 2021 •

edited

Loading

mkbrewer Jun 20, 2021 •

edited

Loading