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

Consistent norm computation#1442

Merged
astrojuanlu merged 2 commits into
poliastro:mainfrom
Yash-10:fast-norm
Jan 15, 2022
Merged

Consistent norm computation#1442
astrojuanlu merged 2 commits into
poliastro:mainfrom
Yash-10:fast-norm

Conversation

@Yash-10

@Yash-10 Yash-10 commented Jan 15, 2022

Copy link
Copy Markdown
Member

From observations in #1433.

Thanks!

@Yash-10

Yash-10 commented Jan 15, 2022

Copy link
Copy Markdown
Member Author

Trying to investigate the docs error...

@codecov

codecov Bot commented Jan 15, 2022

Copy link
Copy Markdown

Codecov Report

Merging #1442 (b489ec6) into main (9797eb0) will decrease coverage by 0.09%.
The diff coverage is 100.00%.

❗ Current head b489ec6 differs from pull request most recent head d01ad0b. Consider uploading reports for the commit d01ad0b to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1442      +/-   ##
==========================================
- Coverage   91.93%   91.83%   -0.10%     
==========================================
  Files          93       95       +2     
  Lines        4400     4448      +48     
  Branches      422      430       +8     
==========================================
+ Hits         4045     4085      +40     
- Misses        267      273       +6     
- Partials       88       90       +2     
Impacted Files Coverage Δ
src/poliastro/_math/linalg.py 100.00% <100.00%> (ø)
src/poliastro/core/czml_utils.py 100.00% <100.00%> (ø)
src/poliastro/core/elements.py 89.62% <100.00%> (ø)
src/poliastro/core/events.py 100.00% <100.00%> (ø)
src/poliastro/core/flybys.py 100.00% <100.00%> (ø)
src/poliastro/core/iod.py 90.00% <100.00%> (ø)
src/poliastro/core/maneuver.py 100.00% <100.00%> (ø)
src/poliastro/core/perturbations.py 100.00% <100.00%> (ø)
src/poliastro/core/propagation/vallado.py 87.50% <100.00%> (+0.40%) ⬆️
src/poliastro/core/spheroid_location.py 100.00% <100.00%> (ø)
... and 10 more

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 9797eb0...d01ad0b. Read the comment docs.

@Yash-10

Yash-10 commented Jan 15, 2022

Copy link
Copy Markdown
Member Author

It seems if the input array contains extra unwanted dimensions, it fails. np.squeeze() is not yet an option in numba, so maybe something else is needed.

(Maybe support to different types of norms, i.e. L1, L2, etc could also be kept just as in the case of np.linalg.norm)

@astrojuanlu

astrojuanlu commented Jan 15, 2022

Copy link
Copy Markdown
Member

I see that norm(Ephem.from_body(Venus, flyby_1_time).rv()[0]) fails, because the resulting vector has ndim == 2. On the other hand, both norm(Ephem.from_body(Venus, flyby_1_time).rv()[0][0]) and norm(Ephem.from_body(Venus, flyby_1_time).rv(flyby_1_time)[0]) succeed.

We want norms of vectors, not norm of matrices, so I think this failure is correct. It forces the user to put the vector in the correct shape though. Addmittedly, Ephem.from_body(Venus, flyby_1_time).rv(flyby_1_time)[0] is not a very convenient way of computing the position of a planet at a given time, we are probably overusing Ephem there. But I suggest we make the small change in the notebook and we continue.

@astrojuanlu astrojuanlu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks perfect to me! When we fix the notebook that is failing, we can merge.

Comment thread src/poliastro/_math/linalg.py Outdated
Add fast norm

reformat

c

d

change
@astrojuanlu
astrojuanlu merged commit 041525f into poliastro:main Jan 15, 2022
@astrojuanlu

Copy link
Copy Markdown
Member

Thanks a lot @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.

2 participants