Skip to content
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

3D maneuvers and docstrings #599

Merged
merged 6 commits into from Mar 14, 2019
Merged

3D maneuvers and docstrings #599

merged 6 commits into from Mar 14, 2019

Conversation

@jorgepiloto
Copy link
Member

@jorgepiloto jorgepiloto commented Mar 13, 2019

This PR enables to compute the different maneuvers (Hohmann and Bielliptic) no matter the inclination of the orbit. Also the corresponding docstrings have been included for each function.

I reopened due to an strange error in previous PR.

@ghost ghost assigned jorgepiloto Mar 13, 2019
@ghost ghost added the 2 - In Progress label Mar 13, 2019
@astrojuanlu
Copy link
Member

@astrojuanlu astrojuanlu commented Mar 13, 2019

The reason why Python 3.5 tests are failing is because of numba/llvmlite#471. Options:

  • Wait, because the developers said half an hour ago they are producing llvmlite and numba wheels.
  • Make numba optional, and don't depend on it for Python 3.5 ("in this version your code will be slow, please upgrade")
  • Temporarily limit the version of numba/llvmlite until the "problem" is fixed.

Note that this is not actually a problem: it just makes the installation more cumbersome (ah, the good old times without wheels...).

Loading

@ghost ghost assigned astrojuanlu Mar 13, 2019
@astrojuanlu
Copy link
Member

@astrojuanlu astrojuanlu commented Mar 13, 2019

My sorry attempt to fix this in a quick way failed. Will continue in another PR, sorry for the noise.

Loading

t_trans2 = (np.pi * np.sqrt((r_i * (R + Rs) / 2) ** 3 / k)).decompose()
r_i = orbit_i.r
v_i = orbit_i.v
h_i = norm(cross(r_i.to(u.m), v_i.to(u.m / u.s)) * u.m ** 2 / u.s)
Copy link
Member

@astrojuanlu astrojuanlu Mar 13, 2019

Choose a reason for hiding this comment

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

Suggested change
h_i = norm(cross(r_i.to(u.m), v_i.to(u.m / u.s)) * u.m ** 2 / u.s)
h_i = norm(cross(r_i.to(u.m), v_i.to(u.m / u.s)))

Loading

Copy link
Member Author

@jorgepiloto jorgepiloto Mar 13, 2019

Choose a reason for hiding this comment

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

I tried to supress those units as suggested but then the tests did not passed, since the following exception is raised:

---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-15-1ae6b7d64845> in <module>
      1 orbit_i = Orbit.circular(Earth, alt=191.34411 * u.km, inc=0*u.deg)
----> 2 hohmann(orbit_i, r_f=35781.34857*u.km + Earth.R)

<ipython-input-14-a8a7f89cc4a4> in hohmann(orbit_i, r_f)
     29 
     30     #h_i = norm((cross(r_i.to(u.m), v_i.to(u.m / u.s)))) * u.m ** 2 / u.s
---> 31     h_i = norm(cross(r_i.to(u.m), v_i.to(u.m / u.s)))
     32     print(h_i)
     33     p = h_i ** 2 / k.to(u.m**3 / u.s**2)

~/Documentos/Github/poliastro/src/poliastro/util.py in norm(vec)
     79 
     80     """
---> 81     return norm_fast(vec.value) * vec.unit
     82 
     83 

AttributeError: 'numpy.ndarray' object has no attribute 'value'

Loading

Copy link
Member

@astrojuanlu astrojuanlu Mar 13, 2019

Choose a reason for hiding this comment

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

Oops, perhaps * u.m ** 2 / u.s has to be outside of the parentheses?

Loading

Copy link
Member Author

@jorgepiloto jorgepiloto Mar 13, 2019

Choose a reason for hiding this comment

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

There we go! The thing was with the core functions. Do you think it would be interesting to implement them (rv_pqw, pqw2ijk, cross) in the high level api, like for example the util.norm function?

Loading

Copy link
Member

@astrojuanlu astrojuanlu Mar 14, 2019

Choose a reason for hiding this comment

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

Maybe! Let's leave that for a future PR :)

Loading

@codecov
Copy link

@codecov codecov bot commented Mar 13, 2019

Codecov Report

Merging #599 into master will increase coverage by 0.08%.
The diff coverage is 95.34%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #599      +/-   ##
==========================================
+ Coverage   84.95%   85.03%   +0.08%     
==========================================
  Files          51       51              
  Lines        2360     2386      +26     
  Branches      181      183       +2     
==========================================
+ Hits         2005     2029      +24     
  Misses        308      308              
- Partials       47       49       +2
Impacted Files Coverage Δ
src/poliastro/maneuver.py 94.44% <95.34%> (-1.21%) ⬇️

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 c6e5b4a...c175acb. Read the comment docs.

Loading

1 similar comment
@codecov
Copy link

@codecov codecov bot commented Mar 13, 2019

Codecov Report

Merging #599 into master will increase coverage by 0.08%.
The diff coverage is 95.34%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #599      +/-   ##
==========================================
+ Coverage   84.95%   85.03%   +0.08%     
==========================================
  Files          51       51              
  Lines        2360     2386      +26     
  Branches      181      183       +2     
==========================================
+ Hits         2005     2029      +24     
  Misses        308      308              
- Partials       47       49       +2
Impacted Files Coverage Δ
src/poliastro/maneuver.py 94.44% <95.34%> (-1.21%) ⬇️

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 c6e5b4a...c175acb. Read the comment docs.

Loading

@astrojuanlu astrojuanlu merged commit 0433a1d into poliastro:master Mar 14, 2019
9 checks passed
Loading
@ghost ghost removed the 2 - In Progress label Mar 14, 2019
@astrojuanlu
Copy link
Member

@astrojuanlu astrojuanlu commented Mar 14, 2019

Thanks a lot @jorgepiloto! It took a lot of time but it was worth it 🎉

Loading

@jorgepiloto jorgepiloto deleted the maneuver branch Mar 14, 2019
@astrojuanlu
Copy link
Member

@astrojuanlu astrojuanlu commented Jul 20, 2019

@jorgepiloto I notice we didn't add any new tests to this pull request. Can you please find an example of a Hohmann transfer that could not be done before it, that now works?

Loading

@astrojuanlu astrojuanlu mentioned this pull request Apr 12, 2020
@astrojuanlu astrojuanlu mentioned this pull request Apr 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants