Skip to content

Conversation

luigigisolfi
Copy link
Contributor

@luigigisolfi luigigisolfi commented Oct 14, 2025

This PR solves (sub)issue #504 by adding an 80 columns format parser, based on the object_type (can be comet, minor_planet, natural_satellite).
It returns an astropy table ready to be validated through the from_astropy() method, and ready to be converted into an observation collection via the to_tudat() method.

It also solves (sub)issue #503 by making the ephemeris() function of the Horizons Wrapper more robust.

@luigigisolfi luigigisolfi added the type: enhancement Improvements to existing functionality label Oct 14, 2025
@luigigisolfi luigigisolfi moved this to Pull request in Tudat development board Oct 14, 2025
@luigigisolfi luigigisolfi changed the title Feature/80col parser Feature/80col parser (closes sub-issue #504) Oct 14, 2025
@luigigisolfi luigigisolfi changed the title Feature/80col parser (closes sub-issue #504) Feature/80col parser (closes sub-issues #504 & #505) Oct 14, 2025
@DominicDirkx
Copy link
Member

Awesome! Have you checked whether the unit tests still pass with these modications?

@luigigisolfi
Copy link
Contributor Author

Awesome! Have you checked whether the unit tests still pass with these modications?

Answer: yes :D

@DominicDirkx
Copy link
Member

Could you also add a test where the 80-column reading is checked?

@luigigisolfi
Copy link
Contributor Author

luigigisolfi commented Oct 16, 2025

Hi @DominicDirkx!

It took me some time, but I think now everything works fine. In the end, this PR turned out to be much bigger than it initially was. New additions:

  1. mpc.py now contains the class MPC80ColsParser, which promises to do what its name suggests
  2. following up on [F&E]: Time Conversion Issues for Legacy MPC Observations #507, the following files were modified (with the help of Gemini, since I am no C++ master):
  • include/tudat/astro/basic_astro/dateTime.h : applied workaround for dates before UNIX dates (1,1,1970 for Win and 1,1,1900 for Apple)
  • include/tudat/astro/earth_orientation/terrestrialTimeScaleConverter.h: now using tudat's historical interpolator for older dates
  1. added unit tests for the main functionalities of MPC80ColsParser
  2. Fixed some inconsistencies withinhorizons_wrapper.py and changed the MPC unit tests and Horizons unit tests (both using Horizons Wrapper) accordingly

I would ask you (and maybe @valeriof7 , since I have touched some time conversion source code) to review the PR.

The only thing I am not very happy about is the test_parse_80cols_file() unit test: the differences in times between the queried table from astroquery and the 80col parsed one, are on the level of 1e-5 seconds (with some of them being 1e-6 or 0.0, though). Not sure where this difference arises, but I think we can leave it for now, and open an issue about it.

@DominicDirkx
Copy link
Member

DominicDirkx commented Oct 16, 2025

Thanks for the thorough update of the mpc code and time conversions! This resolves a lot of small ongoing issues :)

I won't be able to look at this before going on holiday, but I recal @larshinueber also working on the time conversions recently. And, since I'm no Python programmer, could one of you (@valeriof7, @larshinueber, @alfonsoSR) do a check of the Python code?

I see one time conversion unit test is failing:

2025-10-16T04:35:18.2512140Z unknown location(0): �[4;31;49mfatal error: in "test_date_time/testTimePointConversions": std::runtime_error: Failed to convert date to time_t using mktime for date: 1969-12-31 23:59:59.000000000000000�[0;39;49m
2025-10-16T04:35:18.2513068Z /home/runner/work/tudatpy/tudatpy/tests/test_tudat/src/astro/basic_astro/unitTestDateTime.cpp(262): �[1;36;49mlast checkpoint�[0;39;49m

which is an edge case of a time comversion very close to 1-1-1970

And, now that the C++ code is made more robust for time scale conversions in teh more distant past, also important to extend the unit test for this

@alfonsoSR
Copy link
Contributor

I'll have a look at the code this weekend :)

@luigigisolfi
Copy link
Contributor Author

Oh, I know why the test is failing. I'll fix it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: enhancement Improvements to existing functionality

Projects

Status: Pull request

Development

Successfully merging this pull request may close these issues.

3 participants