Skip to content
This repository was archived by the owner on Oct 14, 2023. It is now read-only.

Refactor math (was: poliastro.ephem cleanup) #1434

Merged
astrojuanlu merged 9 commits into from
Jan 9, 2022
Merged

Refactor math (was: poliastro.ephem cleanup) #1434

astrojuanlu merged 9 commits into from
Jan 9, 2022

Conversation

@astrojuanlu
Copy link
Member

@astrojuanlu astrojuanlu commented Dec 27, 2021 •
edited
Loading

Notable changes:

  • Rewrite build_ephem_interpolant in terms of existing classes. I tried to remove its usage but it will require a bit more care. Had to revert this because it caused a massive slowdown, see below.
  • Embrace the Astropy builtin ephemeris everywhere in the tests, now that Astropy >= 5.0 can compute the Moon velocity! Use new erfa routine to calculate the position of the Moon astropy/astropy#11753 The large diff in the tests is because of the indentation removal, should be mostly a whitespace change.
  • As a result, drop support for Python 3.7.
  • Create new poliastro._math package with all mathematical functions not strictly related to Astrodynamics, so we can easily identify (and possibly refactor) dependencies with SciPy. I decided to make it private but maybe we shouldn't be so overprotective, I'm open to suggestions... 😋

@astrojuanlu
Copy link
Member Author

astrojuanlu commented Dec 27, 2021

Pro tip:

Screenshot 2021-12-27 at 07-46-57 poliastro ephem cleanup by astrojuanlu · Pull Request #1434 · poliastro poliastro

@astrojuanlu
Copy link
Member Author

astrojuanlu commented Dec 27, 2021

Next in line: astrojuanlu#13

@astrojuanlu
Copy link
Member Author

astrojuanlu commented Dec 27, 2021

Hmmm slow tests got really slow? 🤔

@astrojuanlu astrojuanlu reopened this Dec 27, 2021
@astrojuanlu
Copy link
Member Author

astrojuanlu commented Jan 8, 2022

py38-slow took ~12 minutes on main, let's see if there's a slowdown. I did some tests and I think there shouldn't, but we'll see.

@astrojuanlu astrojuanlu enabled auto-merge January 8, 2022 16:39
astrojuanlu
astrojuanlu commented Jan 9, 2022
View reviewed changes
src/poliastro/ephem.py Outdated
ephem = Ephem.from_body(body, epochs, attractor=attractor)

r_values[i] = r.xyz.to(u.km)
def interpolant(t):
Copy link
Member Author

@astrojuanlu astrojuanlu Jan 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This probably poses a big performance regression and can't be merged as it stands now 😭

@astrojuanlu
Copy link
Member Author

astrojuanlu commented Jan 9, 2022

I have some ideas on how I could tackle this but I'll need more time. For now, I'll partially revert this and merge.

@astrojuanlu
Copy link
Member Author

astrojuanlu commented Jan 9, 2022

Notes: https://github.com/poliastro/poliastro/wiki/Ideas-for-fast-interpolation

@astrojuanlu astrojuanlu changed the title poliastro.ephem cleanup Refactor math (was: poliastro.ephem cleanup) Jan 9, 2022
@astrojuanlu
Copy link
Member Author

astrojuanlu commented Jan 9, 2022

Looking much better now.

@astrojuanlu astrojuanlu disabled auto-merge January 9, 2022 19:18
@astrojuanlu astrojuanlu merged commit 7746b13 into poliastro:main Jan 9, 2022
@astrojuanlu astrojuanlu mentioned this pull request Jan 9, 2022
Merged
@astrojuanlu astrojuanlu deleted the ephem-cleanup branch January 14, 2022 09:48
@astrojuanlu astrojuanlu mentioned this pull request Jan 14, 2022
Merged
@astrojuanlu astrojuanlu mentioned this pull request Feb 19, 2022
Open
@astrojuanlu astrojuanlu mentioned this pull request Jul 10, 2022
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@jorgepiloto jorgepiloto Awaiting requested review from jorgepiloto

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@astrojuanlu