Remove timescale from FITS format time strings #7870
Conversation
|
Hi there @timj 👋 - 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. |
timj
force-pushed
the
u/timj/fits-time
branch
from
02c7afa
to
ea7a55b
Compare
|
This concerns mostly astropy.time so I will leave it to @mhvk 😉 |
There was a problem hiding this comment.
Thanks for the PR! I think it would be preferable to avoid hard crashes in cases where the scale was included in the brackets. Could you make it so that parsing e.g:
t = Time('2010-01-01T00:00:34.000(TAI)', format='fits')
still works but emits a DeprecationWarning? You can even just ignore the content of the brackets rather than parsing it, but just at least explicitly saying that this part is ignored, so that things don't crash and the user has some feedback.
Then t.fits would of course return the value without the scale.
If you can fix this up, I think it would be great to include this in v3.1 to minimize the number of files with issues.
astropy/time/formats.py
Outdated
|
|
||
| ISOT with two extensions: | ||
| ISOT with one extension: | ||
| - Can give signed five-digit year (mostly for negative years); |
There was a problem hiding this comment.
This now reads strangely with only one bullet point - can you just make it a sentence instead?
|
👍 To keeping the parsing working with a deprecation warning. |
|
Ok. I'll have another stab over the weekend. |
|
@timj - thanks! Just so you know, the feature freeze for v3.1 is at the end of tomorrow, so if there's any way at all you could wrap this up by then, we could be sure to include it in v3.1. There may be a small chance of getting it in over the weekend depending on when exactly the branching of v3.1.x happens though, in case you can't finish it by tomorrow evening. |
|
Personally, I would classify this as "breaking bug fix" 😛 |
timj
force-pushed
the
u/timj/fits-time
branch
2 times, most recently
from
5d2202f
to
5c255b6
Compare
|
I've updated the PR so that it now understands the old style (and will parse it) but issues a deprecation warning (I added a test for it). Newly created strings will not have the time scale. |
|
👍 |
There was a problem hiding this comment.
Sorry for being so late to the game, but I'm in China at a conference and cannot really keep up with all the feature freeze PRs!
I think this PR is a bit incomplete, since it now no longer seems possible to give the FITS specific timescales and thus, e.g., I think _fits_realization is not used any more. Possibly all that is needed is to add a TODO comment that once the deprecation warning is removed, the rest should be cleaned up as well. Or to allow the FITS scales and representationsto be given in scale? (Or is that possible, I wrote the code, but don't recall and have to run now)
|
I've added some extra tests to make sure things are really working and it seems fine. I've mentioned that the code section is deprecated. I could remove the _fits_realization and _fits_scale attributes since they are now ignored, but since this code will all be deleted in the future it didn't seem worth doing. I'll have a quick go at removing those attributes. I agree that there is a problem now that there is no way to use the FITS deprecated timescales. The problem seems to be that: doesn't work because the scale is handled above the formatter and is rejected before the FITS class has a chance to do anything. Fixing this would require that the Time constructor asks the formatter for a timescale translation. Not something for this PR I feel. |
|
@timj - thanks for enhancing the tests - can you rebase this PR to get rid of the merge conflict in the changelog? |
The FITS standard does not allow for the timescale to be included at the end of the ISO format string.
Adding tests for scale consistency.
timj
force-pushed
the
u/timj/fits-time
branch
from
a9c26e6
to
071a371
Compare
|
Rebased and I've also removed the fits realization code and added tests that things still work the same. |
|
Thank you @timj! |
|
Thanks @timj! |
As discussed in #7781 and #5329.
It might be better to retain the parsing support for timescale but no longer include the timescale when converting to a string.