Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes ingestion of IERS EOP 1980 finals.data.csv files after Nov. 6 2023 #3

Merged
merged 3 commits into from
Nov 6, 2023

Conversation

ThatcherC
Copy link
Contributor

See https://datacenter.iers.org/data/2/message_490.txt - four new columns are added the CSV. This MR just bumps the index numbers of all columns after column 9 by 4 to account for the new columns in the middle of the CSV.

A better fix might be to either

  • index columns by name instead of number (in case more columns are added in the future), or
  • switch the to the JSON format of the file mentioned in Message 490. That might be easier to parse and give more flexibility in case more columns are added in the future

See https://datacenter.iers.org/data/2/message_490.txt - four new
columns are added the CSV. This commit bumps the index numbers of all
columns after column 9 by 4 to account for the new columns in the middle
of the CSV.
@ThatcherC ThatcherC changed the title Fixes ingestion of IERS EOP 1980 finals.data.csv files afte Nov. 6 2023 Fixes ingestion of IERS EOP 1980 finals.data.csv files after Nov. 6 2023 Nov 6, 2023
@ThatcherC
Copy link
Contributor Author

All tests are passing locally now! Looks like the 2000A file format also changed so I've added a little fix for that too.

@ThatcherC
Copy link
Contributor Author

I also have a version of this in a branch - https://github.com/ThatcherC/SatelliteToolboxTransformations.jl/tree/eop-from-column-names - that resolves this issue by referencing the EOP columns by name rather than by index, which I think is a little more clear and resilient to future file format changes.

Here's the main commit with that new approach - ThatcherC@47f7f95. I could open an PR for that instead of this change if there's interest!

Copy link

codecov bot commented Nov 6, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (722a7e1) 100.00% compared to head (ad4d792) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##              main        #3   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           30        30           
  Lines         1160      1164    +4     
=========================================
+ Hits          1160      1164    +4     
Files Coverage Δ
src/eop/private.jl 100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ronisbr
Copy link
Member

ronisbr commented Nov 6, 2023

Hi @ThatcherC !

That's awesome! Thanks!!

I also have a version of this in a branch - https://github.com/ThatcherC/SatelliteToolboxTransformations.jl/tree/eop-from-column-names - that resolves this issue by referencing the EOP columns by name rather than by index, which I think is a little more clear and resilient to future file format changes.

IMHO, your verification of the number of columns should be fine for now! Those formats are very stable. I think the best effort would be to change the parser to accept the JSON file as you proposed. This version will be as future proof as we can get. My only concert is that it might take much longer to parse compared to the current format.

@ronisbr ronisbr merged commit ef5f380 into JuliaSpace:main Nov 6, 2023
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants