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

Added tests for rot_elements_at_epoch function #602

Open
wants to merge 13 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@amsuhane
Copy link

commented Mar 16, 2019

Added tests for rot_elements_at_epoch function

@Juanlu001

This comment has been minimized.

Copy link
Member

commented Mar 17, 2019

Hi @amsuhane, thanks for this pull request! I will need some more time to read the tests because they are a bit long, but in the meanwhile could you please fix the import order? The "quality" step is failing because of that reason. You can apply isort yourself using isort --recursive --project poliastro --section-default THIRDPARTY src setup.py (or, if you have poliastro installed in your development environment, which is the usual thing to do, just isort --recursive src).

@codecov

This comment has been minimized.

Copy link

commented Mar 17, 2019

Codecov Report

Merging #602 into master will increase coverage by 2.47%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #602      +/-   ##
==========================================
+ Coverage   85.03%   87.51%   +2.47%     
==========================================
  Files          51       51              
  Lines        2386     2386              
  Branches      183      183              
==========================================
+ Hits         2029     2088      +59     
+ Misses        308      249      -59     
  Partials       49       49
Impacted Files Coverage Δ
src/poliastro/bodies.py 98.99% <100%> (+29.64%) ⬆️

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 0433a1d...eef0ffe. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

commented Mar 17, 2019

Codecov Report

Merging #602 into master will increase coverage by 2.41%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #602      +/-   ##
==========================================
+ Coverage   84.87%   87.28%   +2.41%     
==========================================
  Files          51       51              
  Lines        2446     2446              
  Branches      191      191              
==========================================
+ Hits         2076     2135      +59     
+ Misses        319      260      -59     
  Partials       51       51
Impacted Files Coverage Δ
src/poliastro/bodies.py 99.01% <100%> (+28.92%) ⬆️

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 c8f316c...467e3ce. Read the comment docs.

@Juanlu001
Copy link
Member

left a comment

I left some comments, but the tests are so long that in general from a quick overview it's difficult to understand what are we testing. It is only the values of the constants? I'm not sure where does all that logic come from, but it's essential to document it and find out if we can simplify it.

Show resolved Hide resolved setup.py Outdated
ra = (281.01 - 0.033 * T) * u.deg
dec = (61.45 - 0.005 * T) * u.deg
W = (329.548 + 6.1385025 * d) * u.deg
ra = (281.0097 - 0.0328 * T) * u.deg

This comment has been minimized.

Copy link
@Juanlu001

Juanlu001 Mar 18, 2019

Member

Can you please write a link at the top of the file to see where can we get these values from?

This comment has been minimized.

Copy link
@amsuhane

This comment has been minimized.

Copy link
@Juanlu001

Juanlu001 Mar 29, 2019

Member

I think this was not done yet? We should have this information in the code, not only in the tests. Just a line in the top docstring stating where do they come from, as it's done in other modules.

@amsuhane

This comment has been minimized.

Copy link
Author

commented Mar 19, 2019

Orientation models to calculate the rotational elements (ICRF right ascension and declination and prime meridian location) are given in the kernel file "pck00010.tpc", provided by Nasa. These models have been used to calculate the rotational elements.

The kernel file provides the coefficients of polynomials and trigonometric terms of the models. Hence, in the tests, coefficients are taken from the kernel file and plugged into the polynomial/trigonometric terms.

Url for kernel file is https://naif.jpl.nasa.gov/pub/naif/generic_kernels/pck/pck00010.tpc

@amsuhane

This comment has been minimized.

Copy link
Author

commented Mar 19, 2019

@Juanlu001 I am not sure how to correct the error FileNotFoundError: [Errno 2] No such file or directory: '/home/circleci/repo/src/poliastro/tests/kernels/pck00010.tpc'. The tests are running fine locally.

@Juanlu001

This comment has been minimized.

Copy link
Member

commented Mar 19, 2019

The tests are running fine locally.

Perhaps because you already created the directory?

@Juanlu001
Copy link
Member

left a comment

Left only one comment.

Show resolved Hide resolved src/poliastro/tests/test_bodies.py Outdated
@@ -90,9 +91,23 @@ def test_from_relative():


class TestRotElements:

"""Orientation models to calculate the rotational elements (ICRF right ascension

This comment has been minimized.

Copy link
@Juanlu001

Juanlu001 Mar 19, 2019

Member

Perfect!

@Juanlu001

This comment has been minimized.

Copy link
Member

commented Mar 19, 2019

May I request that the only two SPICE experts that I know, @AndrewAnnex and @marcsit, take a look at this pull request when they have the time and will to do so? 😊 It would be great to confirm that we are following the right approach here.

@Juanlu001

This comment has been minimized.

Copy link
Member

commented Mar 19, 2019

I just discovered https://github.com/esaSPICEservice/spiops by the way, in case it's any useful.

@amsuhane

This comment has been minimized.

Copy link
Author

commented Mar 19, 2019

I'll look into spiops. Thanks for the link!

@Juanlu001
Copy link
Member

left a comment

I added some extra comments. Also, this will need a git rebase on current master.

ra = (281.01 - 0.033 * T) * u.deg
dec = (61.45 - 0.005 * T) * u.deg
W = (329.548 + 6.1385025 * d) * u.deg
ra = (281.0097 - 0.0328 * T) * u.deg

This comment has been minimized.

Copy link
@Juanlu001

Juanlu001 Mar 29, 2019

Member

I think this was not done yet? We should have this information in the code, not only in the tests. Just a line in the top docstring stating where do they come from, as it's done in other modules.

BODY599_NUT_PREC_RA = spice.gdpool("BODY599_NUT_PREC_RA", 0, 15)
BODY599_NUT_PREC_DEC = spice.gdpool("BODY599_NUT_PREC_DEC", 0, 15)

Ja = BODY5_NUT_PREC_ANGLES[20] + BODY5_NUT_PREC_ANGLES[21] * T

This comment has been minimized.

Copy link
@Juanlu001

Juanlu001 Mar 29, 2019

Member

I was shocked when I saw the code the first time some days ago, but then I realized it's exactly the same logic we're applying in bodies.py. Perhaps it would be better to have functions that can be reused, so they just receive appropriate numerical values? That would make these tests shorter.

This comment has been minimized.

Copy link
@amsuhane

amsuhane Mar 29, 2019

Author

I'll start working on the functions.

This comment has been minimized.

Copy link
@amsuhane

amsuhane Apr 1, 2019

Author

I tried to write functions that would help in reducing the length of the tests. The problem is that the big tests are unique and there isn't a generalized way of calculating them. So, I'll just end up passing all the coefficients in the functions, which won't really make the tests shorter Instead, I was thinking I could just write them in a more compact like:
new_format
instead of
old

@amsuhane amsuhane force-pushed the amsuhane:tests_bodies_rot_elements branch from d0c34b0 to 2d731d7 Mar 29, 2019

amsuhane added some commits Mar 29, 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.