-
Notifications
You must be signed in to change notification settings - Fork 0
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
Change default sun position method to accurate and other minor improvements #36
Conversation
d0433b2
to
b969bac
Compare
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 have checked the tests and the notebooks. The changes make sense to me.
While you get "just" a 60% improvement from the fast method, in my machine the slow method reliably takes about 10 times the fast one. Running the same notebook, %timeit run_func(ska_sun.position, times)
takes 81 msec, and %timeit run_func(ska_sun.position_fast, times)
takes 8.1 msec.
Still, I do not know what are the use cases to determine whether that is ok.
looking at it a bit better, so I compare similar methods, the following takes 81 msec in the accurate case, amd 16 msec in the fast case
so that's a factor of 5. Still, it can be acceptable. |
@javierggt - huh. First question is whether you had all the latest branches of kadi, chandra_maneuver and chandra_aca installed. There can even be problems with Beyond that, we have sunk so much into this and are so deep that we may have to accept the performance unless it is truly show-stopper. |
Ah! I had the latest chandra_aca, but not the latest kadi and chandra_maneuver, so that can be it. |
Description
With the speed improvements in sot/chandra_aca#162, it is now reasonable to use the
"accurate"
Sun position method as the default. The "accurate" method is only 60% slower than "fast", but end-to-end theattitudes()
function and kadi states generation is still a factor of 2 faster than current flight.The
pitch
,nominal_roll
andoff_nominal_roll
functions now have amethod
kwarg to specify the sun position method.This PR also improves a number of docstrings and converts them to numpydoc format.
Required dependent PRs
Interface impacts
position_at_jd
was renamed toposition_fast_at_jd
to clarify that this function does not respect thesun_position_method_default
configuration value.Testing
Changes the
position_at_jd
function name toposition_fast_at_jd
. The former name was misleading and I lost some time because of thinking it would compute accurace positions.Unit tests
Independent check of unit tests by [REVIEWER NAME]
Functional tests
See the included PR36 notebook.