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

Heliosynchronous orbit calculation added #598

Open
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@sashank27
Copy link

commented Mar 13, 2019

Addresses #514

@sashank27

This comment has been minimized.

Copy link
Author

commented Mar 13, 2019

@Juanlu001 please let me know how to pass all the test cases

@Juanlu001

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

@sashank27 can you close and reopen? I think some builds failed for no reason

@Juanlu001

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

Sorry, it was because of lack of llvmlite and numba wheels in Python 3.5. Should be fixed soon.

Juanlu001 added some commits Mar 13, 2019

Prefer wheels
This avoids CI failures when there is a new version of a dependency
but the wheel was not uploaded to PyPI.
Fix AppVeyor as well
[skip ci]
@Juanlu001

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

@sashank27 can you rebase on current master? I fixed the CI issues.

@codecov

This comment has been minimized.

Copy link

commented Mar 13, 2019

Codecov Report

Merging #598 into master will decrease coverage by 0.55%.
The diff coverage is 15.78%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #598      +/-   ##
=========================================
- Coverage   84.95%   84.4%   -0.56%     
=========================================
  Files          51      51              
  Lines        2360    2379      +19     
  Branches      181     184       +3     
=========================================
+ Hits         2005    2008       +3     
- Misses        308     324      +16     
  Partials       47      47
Impacted Files Coverage Δ
src/poliastro/twobody/orbit.py 85.52% <15.78%> (-4.65%) ⬇️

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 2e26192...11a20ff. Read the comment docs.

@Juanlu001

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

This will need some tests :)

@sashank27

This comment has been minimized.

Copy link
Author

commented Mar 14, 2019

Could you tell me where to get the data so as to create the tests?

@Juanlu001

This comment has been minimized.

Copy link
Member

commented Mar 14, 2019

I would try to look for some textbook example in Curtis or Vallado (see https://docs.poliastro.space/en/stable/references.html#books-and-papers). If not, some paper, blog article, that contains this kind of orbit. And if not, we'll have to use some external software https://github.com/poliastro/poliastro/wiki/Validation

@Juanlu001
Copy link
Member

left a comment

Sorry for the delay! Apart from the lack of tests (both functional and numerical), I left some comments.

@@ -580,6 +580,32 @@ def geostationary(
altitude = geo_radius - attractor.R
return cls.circular(attractor, altitude)

@classmethod
@u.quantity_input(a=u.m, ecc=u.one, w=u.rad / u.s, period=u.s)
def heliocentric(cls, attractor, a, ecc, w=None, period=None):

This comment has been minimized.

Copy link
@Juanlu001

Juanlu001 Mar 18, 2019

Member

Shouldn't this be called heliosynchronous? Heliocentric means a different thing.

This comment has been minimized.

Copy link
@sashank27

sashank27 Apr 14, 2019

Author

Sorry, a typo

if a is None:
raise ValueError("The value of semi major axis is required")
mean_motion = np.sqrt(attractor.k / np.cube(a))
h = a * (1 - ecc ** 2)

This comment has been minimized.

Copy link
@Juanlu001

Juanlu001 Mar 18, 2019

Member

Could you please document what's h and where does the theta and inc values come from?

This comment has been minimized.

Copy link
@jorgepiloto

jorgepiloto Mar 19, 2019

Member

May I suggest that the variable mean_motion could be called n? Since mean_motion is one of the propagation methods that poliastroincludes, it could lead to confusion or coding errors in the future.

This comment has been minimized.

Copy link
@sashank27

sashank27 Apr 14, 2019

Author

h is the semi-latus rectum. inc is the cosine of inclination for a given value of semi-major axis, eccentricity, and time period (in radians). theta is the original angle in degrees.
Also, I will replace mean_motion by n.

theta = (-2 / 3) * ((h / attractor.k) ** 2) * (w / (mean_motion * attractor.J2))
inc = (180 / np.pi) * np.arccos(theta)

raan = (0 * u.deg,)

This comment has been minimized.

Copy link
@Juanlu001

Juanlu001 Mar 18, 2019

Member

I'm not sure why we're using tuples here, these probably make the code crash somewhere.

This comment has been minimized.

Copy link
@Juanlu001

Juanlu001 Mar 18, 2019

Member

That means that, apart from tests validating the numerical aspect, we would need something that at least uses the function.

inc = (180 / np.pi) * np.arccos(theta)

raan = (0 * u.deg,)
argp = (0 * u.deg,)

This comment has been minimized.

Copy link
@Juanlu001

Juanlu001 Mar 18, 2019

Member

Also, why are we setting these values to 0?

This comment has been minimized.

Copy link
@sashank27

sashank27 Apr 14, 2019

Author

I am not sure what would be the value of right ascension of the ascending node, argument of the pericenter, and anomaly. So, I took them as 0.

@Juanlu001

This comment has been minimized.

Copy link
Member

commented Mar 19, 2019

Example 11-2 from Vallado 3rd edition has what we need:

photo5803431081498750838

photo5803431081498750839

@Juanlu001

This comment has been minimized.

Copy link
Member

commented Apr 7, 2019

@sashank27 are you still working on this?

@sashank27

This comment has been minimized.

Copy link
Author

commented Apr 10, 2019

Sorry for the delay. I was caught up in some college work. Will be continuing the work again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.