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

Slightly faster method for newton iteration in E and H calculation #1247

Merged
merged 6 commits into from
Jun 14, 2021

Conversation

Yash-10
Copy link
Member

@Yash-10 Yash-10 commented Jun 11, 2021

Hello, as per #1240, this is an attempt to slightly make the computation faster (even thought both functions were already jitted).

Thanks!

@Yash-10 Yash-10 force-pushed the Faster-convergence-newton-E-and-H branch from 7f6113d to 180d7eb Compare June 11, 2021 14:26
@Yash-10
Copy link
Member Author

Yash-10 commented Jun 11, 2021

As was seen locally, there are slight mismatches. The eccentricity conditions in the hyperbolic case might need to be investigated...


Update 1 - If I change the eccentricity value to something lower than 1.6, the tests pass, however this may not be a good approach to solve those small errors because the current implementation still gives an error (although of magntiude ~1e-20 or lower) if I use ecc=1.1, for example, on test_propagation_hyperbolic_zero_time_returns_same_state (this function was the one showing the small errors earlier).

Also, these errors only happen for certain eccentricity ranges (i.e. around 1.1) but not for others.


Update 2 - The added test for ecc=1.1 does pass. As a result, it might be the local errors (from update 1) are not related to the implementation but just inaccuracies with python floats?

@codecov
Copy link

codecov bot commented Jun 11, 2021

Codecov Report

Merging #1247 (3a0fda7) into main (c1ffbaa) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1247      +/-   ##
==========================================
+ Coverage   91.30%   91.31%   +0.01%     
==========================================
  Files          79       79              
  Lines        4138     4143       +5     
  Branches      359      362       +3     
==========================================
+ Hits         3778     3783       +5     
  Misses        270      270              
  Partials       90       90              
Impacted Files Coverage Δ
src/poliastro/core/angles.py 95.55% <100.00%> (+0.26%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c1ffbaa...3a0fda7. Read the comment docs.

@Yash-10 Yash-10 force-pushed the Faster-convergence-newton-E-and-H branch from f6c9ef4 to ae39be9 Compare June 13, 2021 10:28
@astrojuanlu
Copy link
Member

although of magntiude ~1e-20 or lower

Where exactly was this error appearing? I think it should be acceptable to adapt the test accordingly

Also, could you show a comparison of speed or number of iterations before and after?

@Yash-10 Yash-10 force-pushed the Faster-convergence-newton-E-and-H branch from ae39be9 to b7b4a35 Compare June 13, 2021 13:20
@Yash-10
Copy link
Member Author

Yash-10 commented Jun 13, 2021

Where exactly was this error appearing? I think it should be acceptable to adapt the test accordingly

In test_propagation_hyperbolic_zero_time_returns_same_state


Also, could you show a comparison of speed or number of iterations before and after?

I haven't changed the number of iterations for the newton iteration. For speed improvements, there's a slight improvement. I had done an analysis in #1240 for discussion. Maybe that would give an idea?

The functions are jitted and using the timeit module gives the following rough analysis:
Note: the below "no. of iterations" are the no. of times both the implementations are run using timeit and not the no. of iterations for the newton-raphson method.

  • For 1000 iterations, there is ~0.26 s time improvement.
  • For 10000 iterations, there is ~2.62 s time improvement.
  • For 100000 iterations, there is ~26.08 s improvement.
    Summarizing it in a graph:

image

@Yash-10
Copy link
Member Author

Yash-10 commented Jun 13, 2021

As you say, we could make the implementation look exactly like Vallado's (i.e. change the eccentricty condition to < 1.6 in the hyperbolic case) and modify the tests to prevent any minor errors.

@astrojuanlu
Copy link
Member

Yes, let's do it!

Copy link
Member

@astrojuanlu astrojuanlu left a comment

Choose a reason for hiding this comment

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

Thanks @Yash-10 !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants