Invalid TRPOS<n> keyword not generated for FITS time column#8784
Invalid TRPOS<n> keyword not generated for FITS time column#8784taldcroft merged 7 commits intoastropy:masterfrom
Conversation
|
Please fix PEP 8: I think doc build error is unrelated? |
|
docbuild error is indeed unrelated and should be fixed after a rebase. |
7dbf9ff to
b7ea093
Compare
Codecov Report
@@ Coverage Diff @@
## master #8784 +/- ##
==========================================
+ Coverage 86.7% 86.97% +0.27%
==========================================
Files 403 400 -3
Lines 59965 59461 -504
Branches 1100 1100
==========================================
- Hits 51992 51719 -273
+ Misses 7332 7101 -231
Partials 641 641
Continue to review full report at Codecov.
|
taldcroft
left a comment
There was a problem hiding this comment.
Hi @aaryapatil - sorry for the delay. Here is my first look, please excuse that I have entirely forgotten how your code works and the FITS standard, if I ever understood the first time. 😄
CHANGES.rst
Outdated
| astropy.io.fits | ||
| ^^^^^^^^^^^^^^^ | ||
|
|
||
| - Invalid TRPOS<n> keyword is not generated for FITS time column. [#8784] |
There was a problem hiding this comment.
How about:
Fixes bug where an invalid TRPOS<n> keyword was being generated for FITS
time column when no location was available. [#8784]
astropy/io/fits/fitstime.py
Outdated
| @@ -259,13 +259,20 @@ def _verify_column_info(column_info, global_info): | |||
| # considered. | |||
There was a problem hiding this comment.
This comment is no longer correct, as the code change below also now ignores global reference position. That does not seem like the right thing, but I don't understand the FITS time spec in detail.
astropy/io/fits/fitstime.py
Outdated
| column_info['location'] = global_info['location'] | ||
| if column_info['location'] is None: | ||
| if global_info['location'] is None: | ||
| warnings.warn( |
There was a problem hiding this comment.
I actually don't see why this needs to generate a warning. At least I think this simple case should work without warning:
In [7]: tm = Time(['2000:001'])
...: t = Table([tm])
...: t.write('time.fits', overwrite=True)
...: t2 = t.read('time.fits', astropy_native=True)
...:
...:
WARNING: Time column reference position "TRPOSn" is not specified. The default value for it is "TOPOCENTER", but due to unspecified observatory position, reference position will be ignored. [astropy.io.fits.fitstime]
I don't see any reason to assume that Time data written to FITS has anything to do with an observatory.
There was a problem hiding this comment.
We call is "space-time" because technically time alone does not have a meaning, e.g. if you see two signals in two bands (let's say you discover a supernova in I and R band), you might want to see which band detected it first. Due to the light-travel time, the position of the two observatories is required to do this for small time differences.
However, in practice, most observations are done somewhere on the earth and on a time precision much less than the light travel time, so location does not matter. Thus, I agree that a warning is needed in principle ,but I think it will be annoying for the vast majority of users where it does not matter, so I recommend "no warning".
There was a problem hiding this comment.
I think "no warning" makes sense. I was inclined to atleast warn the user following @hamogu's argument, but it definitely would be annoying.
| 'has been specified. However, for supporting column-specific location, ' | ||
| 'reference position will be ignored for this column.', | ||
| AstropyUserWarning) | ||
| column_info['location'] = None |
There was a problem hiding this comment.
As above, not clear why this behavior is being changed to always ignore global location.
There was a problem hiding this comment.
The problem is when there are multiple time columns in a table, one with a location associated with it and one without one.
t = Table()
t['a'] = Time(['2000:001'], format='isot', scale='utc', location=EarthLocation(-2446354,
4237210, 4077985, unit='m'))
t['b'] = Time([1], format='cxcsec', scale='tt')
The FITS time standard requires us to write the EarthLocation(-2446354, 4237210, 4077985, unit='m') in global time keywords OBSGEO-X, OBSGEO-Y, OBSGEO-Z. TRPOSn is a column-specific keyword and we would set it for the column 'a' as TRPOS1 = 'TOPOCENTER'. But what FITS would do is that if there is no value for TRPOS2 (for column 'b'), by default it would treat it as TRPOS2 = 'TOPOCENTER' and use the global keywords OBSGEO-X, OBSGEO-Y, OBSGEO-Z as its location. But, we want reproducibility with astropy, so instead when TRPOSn is not specified, I treat the default as None instead of 'TOPOCENTER'. I think this makes sense, as the user should ideally specify TRPOSn to make sure there is no ambiguity because of default values.
|
@aaryapatil - If you get there, please rebase this and also remove the capture of the warning from here: https://github.com/astropy/astropy/pull/8754/files#diff-ff07dd01aa949e3fe71d5431d0f7d2bcR167 |
… is not specified
b7ea093 to
4ad4644
Compare
taldcroft
left a comment
There was a problem hiding this comment.
LGTM. Dropping those warnings for typical use cases is good, test changes look good, and I explicitly confirmed that my original issue is fixed. Thanks @aaryapatil !!
Invalid TRPOS<n> keyword not generated for FITS time column
This fixes issue #8773. I had to make a few minor changes to the FITS TIME machinery to handle default values. Have added a small test too!
@taldcroft Do check!
Fixes #8773