-
Notifications
You must be signed in to change notification settings - Fork 14
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
Swap to adam_core's Propagator, Orbits, and Observers #126
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a quick read-through. So nice to clean this up!
num_jobs=1, | ||
chunk_size=1, | ||
) | ||
ephemeris = prop._generate_ephemeris(orbits, observers) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we use the private method here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No good reason, we were calling generate_ephemeris (original function) with num_jobs=1 which would have called the private function in series. I'll go through and replace the private calls throughout IOD, OD, and the arc extension code.
@@ -178,8 +184,8 @@ def iod( | |||
observation_selection_method="combinations", | |||
iterate=False, | |||
light_time=True, | |||
backend="PYOORB", | |||
backend_kwargs={}, | |||
propagator: Literal["PYOORB"] = "PYOORB", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's think about how to make this open enough that people can install third-party packages and use other propagators.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good idea. In range and transform we pass the actual class, not a string. Maybe we pass the class all the way through to the worker functions and then initialize it in there?
propagator: Type["Propagator"] = PYOORB,
propagator_kwargs: dict = {},
Then inside worker functions:
prop = propagator(**propagator_kwargs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll clean all of this up once I have the quivr types built out and can work on the parallelization again.
f5760a7
to
567ecdf
Compare
Yeah, that makes sense to me.
…On Thu, Nov 2, 2023 at 09:48 Joachim Moeyens ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In thor/orbits/iod.py
<#126 (comment)>:
> @@ -178,8 +184,8 @@ def iod(
observation_selection_method="combinations",
iterate=False,
light_time=True,
- backend="PYOORB",
- backend_kwargs={},
+ propagator: Literal["PYOORB"] = "PYOORB",
Yes, good idea. In range and transform we pass the actual class, not a
string. Maybe we pass the class all the way through to the worker functions
and then initialize it in there?
propagator: Type["Propagator"] = PYOORB,propagator_kwargs: dict = {},
Then inside worker functions:
prop = propagator(**propagator_kwargs)
—
Reply to this email directly, view it on GitHub
<#126 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAFTGJF3TLUV3CSQQ3VUAGLYCOQEPAVCNFSM6AAAAAA62JRC3KVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOMJQGM2DOMBWGI>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
9a590aa
to
916a31b
Compare
f12f787
to
b2b837f
Compare
ecdbd27
to
dd85a73
Compare
dd85a73
to
88e6162
Compare
Replaces:
Removes:
All unit tests pass, but most importantly:
pytest -k ivezic -m integration
passes! This tests the entire pipeline.The benchmark shows a slowdown from 1.2695 to 1.4256 seconds. This is more than likely due to the conversion from adam_core types to dataframes still used by a large part of the THOR pipeline. I'll change those types out in a follow up PR.