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

Add a GeocentricSolarEcliptic Frame #562

Merged
merged 10 commits into from Apr 20, 2019

Conversation

Projects
None yet
2 participants
@shreyasbapat
Copy link
Member

commented Feb 3, 2019

Addresses #335

@codecov

This comment has been minimized.

Copy link

commented Feb 3, 2019

Codecov Report

Merging #562 into master will increase coverage by 0.14%.
The diff coverage is 96.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #562      +/-   ##
==========================================
+ Coverage   86.96%   87.11%   +0.14%     
==========================================
  Files          52       52              
  Lines        2509     2537      +28     
  Branches      210      211       +1     
==========================================
+ Hits         2182     2210      +28     
  Misses        260      260              
  Partials       67       67
Impacted Files Coverage Δ
src/poliastro/frames.py 78.15% <96.77%> (+6.72%) ⬆️

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 1d36684...880f529. Read the comment docs.

@shreyasbapat

This comment has been minimized.

Copy link
Member Author

commented Feb 6, 2019

After thinking about it! I have some questions in mind!
In @Juanlu001 's pull request astropy/astropy#6508 he has introduced a transformation matrix which is basically for TrueEcliptic or MeanEcliptic (That is still not clear). But transformation to GSE is very easy in way that Geocentric True Ecliptic and Geocentric solar ecliptic, both share their X-Y Axes in ecliptic plane. I just have to rotate the coordinate axes by an angle which is angle between earth-sun and earth-equinox(true).
First, how do I get that angle?
If I don't have to get that angle, from where do I get the transformation matrix? Does ERFA has such a matrix? I assume no?

@shreyasbapat shreyasbapat changed the title WIP - Add a GeocentricSolarEcliptic Frame [WIP] Add a GeocentricSolarEcliptic Frame Feb 6, 2019

@Juanlu001

This comment has been minimized.

Copy link
Member

commented Feb 8, 2019

Okay, now I understand the problem.

Does ERFA has such a matrix? I assume no?

I am not sure. I looked at it very quickly and didn't see it. We should look more closely.

If I don't have to get that angle, from where do I get the transformation matrix?

Even if it's not in ERFA, I see some formulas here and there that use the "Sun ecliptic longitude":

https://www.spenvis.oma.be/help/background/coortran/coortran.html#T2

However, a random website is not enough and I'd prefer to look at them from some source.

@shreyasbapat

This comment has been minimized.

Copy link
Member Author

commented Feb 8, 2019

@Juanlu001

This comment has been minimized.

Copy link
Member

commented Feb 10, 2019

This will need a rebase now

@shreyasbapat

This comment has been minimized.

Copy link
Member Author

commented Feb 17, 2019

Sorry for a looooong break. I will start working on it

@shreyasbapat

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2019

@Juanlu001

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

Take a look at https://docs.sunpy.org/en/stable/api/sunpy.coordinates.transformations.hcrs_to_hgs.html and https://github.com/sunpy/sunpy/blob/master/sunpy/coordinates/transformations.py#L304 first, and try to understand if we can reuse part of that transformation. Notice that we don't intend to depend on SunPy though.

@shreyasbapat

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2019

This is helpful!

@shreyasbapat

This comment has been minimized.

Copy link
Member Author

commented Mar 22, 2019

Back to it finally! Rebased and started exploring sunpy links!

@shreyasbapat shreyasbapat force-pushed the shreyasbapat:frame branch from 434762b to db541a0 Mar 22, 2019

Show resolved Hide resolved src/poliastro/frames.py Outdated
return matrix


_EARTH_NORTH_POLE_GCRS = UnitSphericalRepresentation(lon=0 * u.deg, lat=66.5 * u.deg)

This comment has been minimized.

Copy link
@shreyasbapat

shreyasbapat Mar 22, 2019

Author Member

I figured out what is correct. Can you please verify this?

@shreyasbapat shreyasbapat changed the title [WIP] Add a GeocentricSolarEcliptic Frame Add a GeocentricSolarEcliptic Frame Mar 22, 2019

" Frame needs an obstime Attribute"
)

sun_pos_icrs = get_body_barycentric("sun", to_frame.obstime)

This comment has been minimized.

Copy link
@shreyasbapat

shreyasbapat Mar 22, 2019

Author Member

And would changing the frame of this matter? As we are finding the difference vector

@Juanlu001
Copy link
Member

left a comment

I left some comments. I think we have to rework the transformation to make it simpler (if possible) by reusing existing systems and functions, in particular (I suspect) ecliptic coordinates.



_EARTH_ORBIT_PERPEN_POINT_GCRS = UnitSphericalRepresentation(
lon=0 * u.deg, lat=66.5 * u.deg

This comment has been minimized.

Copy link
@Juanlu001

This comment has been minimized.

Copy link
@shreyasbapat

shreyasbapat Apr 5, 2019

Author Member

So, I will try obtaining it fro ERFA!

Show resolved Hide resolved src/poliastro/frames.py Outdated
earth_pos_gcrs = get_body("earth", to_frame.obstime).cartesian
sun_earth = sun_pos_gcrs - earth_pos_gcrs

sun_earth_detilt = sun_earth.transform(_EARTH_DETILT_MATRIX)

This comment has been minimized.

Copy link
@Juanlu001

Juanlu001 Mar 29, 2019

Member

Isn't this essentially ecliptic coordinates? If I'm right, then this transformation could be made simpler by connecting it to

class HeliocentricEclipticJ2000(BaseEclipticFrame):
"""
Heliocentric ecliptic coordinates. These origin of the coordinates are the
center of the sun, with the x axis pointing in the direction of
the mean equinox of J2000 and the xy-plane in the plane of the
ecliptic of J2000 (according to the IAU 1976/1980 obliquity model).
"""
obstime = TimeAttribute(default=DEFAULT_OBSTIME)

This comment has been minimized.

Copy link
@shreyasbapat

shreyasbapat Apr 5, 2019

Author Member

I will have a look asap

This comment has been minimized.

Copy link
@shreyasbapat

shreyasbapat Apr 16, 2019

Author Member

I did the GCRS to GSE transformation, and I guess it's easier

@shreyasbapat shreyasbapat force-pushed the shreyasbapat:frame branch 2 times, most recently from 6f975cd to a66cc75 Apr 16, 2019

@shreyasbapat

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2019

Done!

@Juanlu001
Copy link
Member

left a comment

Great job! 👏

I left 1 super minor comment. On the other hand, I acknowledge that adding tests for this is difficult, so can we at least have

  1. A round-trip to/from GCRS, and
  2. A regression test? For example: you run the transformation, obtain some values, copy paste them in a test, make it pass. That way, if we ever change anything, we'll notice.
Show resolved Hide resolved src/poliastro/frames.py Outdated
@shreyasbapat

This comment has been minimized.

Copy link
Member Author

commented Apr 16, 2019

Sure thing. Will do it tonight!

@shreyasbapat

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

Codeclimate will never pass :/ We will have to supress it.

@Juanlu001

This comment has been minimized.

Copy link
Member

commented Apr 17, 2019

According to Codecov, there are two untested branches:

https://codecov.io/gh/poliastro/poliastro/compare/e1b893c11764d301f584071cd50573e9a6947e93...c32b516398de55d9008c1c7d057ef3774c0e7ce1/diff

  • obstime=None
  • obstime not scalar

Please? 😇

@shreyasbapat

This comment has been minimized.

Copy link
Member Author

commented Apr 20, 2019

According to Codecov, there are two untested branches:
https://codecov.io/gh/poliastro/poliastro/compare/e1b893c11764d301f584071cd50573e9a6947e93...c32b516398de55d9008c1c7d057ef3774c0e7ce1/diff
obstime=None
obstime not scalar
Please?

Just realised, astropy infrastructure doesn't allow us to pass None anyways. It defaults to J2000 automatically.

@shreyasbapat shreyasbapat force-pushed the shreyasbapat:frame branch 2 times, most recently from a66ee45 to 4b8bf94 Apr 20, 2019

@shreyasbapat shreyasbapat force-pushed the shreyasbapat:frame branch from 4b8bf94 to 880f529 Apr 20, 2019

@Juanlu001 Juanlu001 merged commit 3ac2cfc into poliastro:master Apr 20, 2019

10 checks passed

ci/circleci: coverage Your tests passed on CircleCI!
Details
ci/circleci: docs Your tests passed on CircleCI!
Details
ci/circleci: quality Your tests passed on CircleCI!
Details
ci/circleci: test_py35 Your tests passed on CircleCI!
Details
ci/circleci: test_py36 Your tests passed on CircleCI!
Details
ci/circleci: test_py37 Your tests passed on CircleCI!
Details
codeclimate All good!
Details
codecov/patch 96.77% of diff hit (target 86.96%)
Details
codecov/project 87.11% (+0.14%) compared to 1d36684
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@wafflebot wafflebot bot removed the 2 - In Progress label Apr 20, 2019

@Juanlu001

This comment has been minimized.

Copy link
Member

commented Apr 20, 2019

😍 Thanks a lot!

@shreyasbapat shreyasbapat deleted the shreyasbapat:frame branch Apr 20, 2019

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.