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

Adds basic CZML extractor #601

Merged
merged 13 commits into from Mar 27, 2019

Conversation

Projects
None yet
2 participants
@Sedictious
Copy link
Contributor

Sedictious commented Mar 15, 2019

This is a draft CZML extractor. It currently can:

  • Generate the orbital CZML data for Cesium's Sandcastle
  • Edit the label, path and id details (text, fonts, colours)
  • Allows for an arbitary number of orbits to be defined

Update:

  • The reference frames now default to inertial

Inertial

@Sedictious Sedictious marked this pull request as ready for review Mar 15, 2019

@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 15, 2019

Codecov Report

Merging #601 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #601   +/-   ##
=======================================
  Coverage   84.87%   84.87%           
=======================================
  Files          51       51           
  Lines        2446     2446           
  Branches      191      191           
=======================================
  Hits         2076     2076           
  Misses        319      319           
  Partials       51       51

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 a8c30b7...90d6b95. Read the comment docs.

@Juanlu001
Copy link
Member

Juanlu001 left a comment

Thanks for this pull request! I gave a quick review but didn't try to run the code or inspect it in depth. Apart from the two minor comments I left, the "quality" step failed, so you might have to apply black and run some other formatters. The best thing is to run tox -e check locally, which will warn about the errors, and apply the corresponding tool without the --check parameter. In the future there will be a tox -e format to automate this.

Show resolved Hide resolved src/poliastro/contrib/extract_czml.py Outdated
Show resolved Hide resolved src/poliastro/contrib/extract_czml.py Outdated

@Sedictious Sedictious force-pushed the Sedictious:CZLM branch 2 times, most recently from 4e96493 to 20c95a1 Mar 15, 2019

@Sedictious

This comment has been minimized.

Copy link
Contributor Author

Sedictious commented Mar 15, 2019

@Juanlu001 Thanks for the review! If I understand correctly, Code climate fails due to "high cyclomatic complexity", which I think refers to the many unnested if statements (if I'm not mistaken, it can have 2^6 different outcomes). Now the solution would be to either "break" the function into two smaller ones or omit a variable altogether.

I'm a bit lost as to why circleci fails. Is it related to the project's dependencies?

Oh and I forgot to ask: How should I raise errors? Is there a logger?

@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Mar 15, 2019

Don't worry about codeclimate for the moment, we can think of ways of simplifying that method in the future (or just ignore it, which is what we do 80 % of the times).

Regarding circleci: we use isort to sort the import statements in a consistent way throughout the project, and it's telling you that the order is wrong:

isort --check-only --diff --recursive --project poliastro --section-default THIRDPARTY src setup.py
ERROR: /root/repo/src/poliastro/contrib/extract_czml.py Imports are incorrectly sorted.
--- /root/repo/src/poliastro/contrib/extract_czml.py:before	2019-03-15 16:16:25.732628
+++ /root/repo/src/poliastro/contrib/extract_czml.py:after	2019-03-15 16:16:37.236302
@@ -1,9 +1,9 @@
+import json
+
 import numpy as np
-import json
-
+from astropy import units as u
+from astropy.coordinates import CartesianRepresentation
 from astropy.time import Time
-from astropy.coordinates import CartesianRepresentation
-from astropy import units as u
 
 from poliastro.contrib.czml_extract_default_params import DEFAULTS
 

Instead of trying to follow that manually, 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).

@Sedictious Sedictious force-pushed the Sedictious:CZLM branch from 3288974 to afbf7ab Mar 15, 2019

@Sedictious

This comment has been minimized.

Copy link
Contributor Author

Sedictious commented Mar 15, 2019

@Juanlu001 Thanks I reformatted and now everything passes except for codeclimate and codecov. Regarding the code coverage, I'm planning on writing tests relatively soon. I'm still undecided about using nested dictionaries for JSON as it seems a bit inelegant, but it's the only efficient and comprehensible way I could come up with.

@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Mar 15, 2019

Thanks I reformatted and now everything passes except for codeclimate and codecov.

Excellent!

I'm still undecided about using nested dictionaries for JSON as it seems a bit inelegant, but it's the only efficient and comprehensible way I could come up with.

I don't have a strong opinion against it, but I would still need to review the code in depth. Give me one day to do it and I will write more meaningful comments and confirm the approach.

@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Mar 18, 2019

I am in process of reviewing this code, and I find it a bit difficult to use. I tried to create a class and then paste its .czml attribute in the Cesium Sandcastle, but it was somehow malformed and I had to slightly change its structure to make it work (namely, convert the result from a dictionary with keys 0 and 1 to a list of dictionaries). Then I found the extract method, which does the right thing. But in general, the class has too many public methods which I suppose are not normally used?

I propose making everything that won't be used by end users private, so perhaps leave only extract, add_orbit and del_orbit (rename to remove_orbit?)

Now I'm making some comments in the code.

@Juanlu001
Copy link
Member

Juanlu001 left a comment

I left some comments. I understand that you introduced parse_dict_tuples and change_*_params to be able to easily modify the parameters in the middle of the process, but I think it makes the code a bit difficult to follow. I propose we do something similar to what our *OrbitPlotter classes do:

  • Have a simple initializer that doesn't take any orbits, only some global parameters (like the default number of points to sample, for example) and that initialize a default template CZML
  • Have methods add_orbit (there) that accept some parameters for customization (instead of change_*_params) and also add_trajectory, which allows the user to pass the result of Orbit.sample directly.
  • Add a write_czml method that takes a file handle and calls json.dump on it

How does that sound?

Show resolved Hide resolved src/poliastro/contrib/extract_czml.py Outdated
Show resolved Hide resolved src/poliastro/contrib/extract_czml.py Outdated
Show resolved Hide resolved src/poliastro/contrib/extract_czml.py Outdated
Show resolved Hide resolved src/poliastro/contrib/extract_czml.py Outdated
Show resolved Hide resolved src/poliastro/contrib/extract_czml.py Outdated
Show resolved Hide resolved src/poliastro/contrib/extract_czml.py Outdated
@Sedictious

This comment has been minimized.

Copy link
Contributor Author

Sedictious commented Mar 20, 2019

@Juanlu001 I fixed most of the things you mentioned (most notably, orbits is now a list, I removed the ability to delete orbits and made DEFAULTS into a dictionary which is simply copied when initializing a new orbit.

What I didn't implement (yet):

  • Changing the way path/text parameters are changed:

I feel like loading up the initializer with so many parameters would be a bit overwhelming. Maybe we could add the "basic" parameters (name, path colour) and keep the other methods as a way to add even more customization options.

  • Using Orbit.sample():

Admittedly, I didn't look too much into the source, but I didn't find the way to set the time values

  • Extract the data to a file

Would a simple f.write(path) suffice?

@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Mar 20, 2019

I feel like loading up the initializer with so many parameters would be a bit overwhelming. Maybe we could add the "basic" parameters (name, path colour) and keep the other methods as a way to add even more customization options.

Perhaps, but take into account that there's lots of parameters in stuff like pandas plotting, or even matplotlib. The tradeoff is: more methods with fewer parameters, or fewer methods with more parameters. I lean towards the latter, to imitate what others do.

Admittedly, I didn't look too much into the source, but I didn't find the way to set the time values

Oops, you're right! Orbit.sample takes a fixed number of points now. The way of doing it now would be:

from poliastro.twobody.propagation import propagate

positions = propagate(
    iss,
    time.TimeDelta(...),
)

(see for example these notebooks [1], [2])

Would a simple f.write(path) suffice?

Use this instead:

with open("results.json", "w") as fp:
    extractor.write_czml(fp)

(and, internally, json.dump on the input fp)

Show resolved Hide resolved src/poliastro/contrib/extract_czml.py Outdated
Show resolved Hide resolved src/poliastro/contrib/extract_czml.py Outdated

@Sedictious Sedictious force-pushed the Sedictious:CZLM branch 2 times, most recently from e9bc6d0 to afee9e9 Mar 23, 2019

@Sedictious

This comment has been minimized.

Copy link
Contributor Author

Sedictious commented Mar 23, 2019

@Juanlu001 I've been thinking about a way to correctly implement the parameter customization. I tried to put everything in one method but it became way too big and hard to follow. Here's the way I'd approach this: Make the parameters_change methods private and call them from the orbit initializer. That way, the user won't get overwhelmed by an abundance of oftentimes redundant methods and the code will still be relatively concise.

@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Mar 23, 2019

@Sedictious

This comment has been minimized.

Copy link
Contributor Author

Sedictious commented Mar 26, 2019

@Juanlu001 So I moved all the additional parameters to add_orbit (now codeclimate has another thing to complain about, yay!) and I think you were right. It is now easier for someone to easily define the parameters they want instead of potentially calling three different methods.

I wanted to ask you: What would the best way of specifying reference frames be? Aren't poliastro's Orbits implicitly inertial unless specified otherwise? If so, is there a way to change said reference system?

@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Mar 26, 2019

Codeclimate is too picky :) I agree it looks easier now!

Aren't poliastro's Orbits implicitly inertial unless specified otherwise?

The fact is that the orbits are internally converted to an inertial reference frame, even when created with the new Orbit.from_coords classmethod. This simplifies propagation and many other things. Therefore, I'd say we always assume it's an inertial system, at least for now.

@Sedictious

This comment has been minimized.

Copy link
Contributor Author

Sedictious commented Mar 26, 2019

@Juanlu001 Oh, I see! I changed the rf to inertial and it now works as expected with your Sandcastle template

Sedictious added some commits Mar 15, 2019

General reformatting
- Initializer doesn't need an Orbit class
- DEFAULTS is now a dictionary
- 'format_date()' uses astropy's converter instead of directly modifying the strings
- initializer methods are now private (well.. in the pythonic sense at least)
- 'orbits' doesn't directly store the period anymore
- removed 'del_orbit()'
- 'orbits' is now a list instead of a dictionary, since there is now no need for deleting anything, objects can be identified by their index.
Fixes general issues
Fixes issue where all the parameters are changed simultaneously
Private modules now have one underscore
`extract` can now extract to a file
Sampling is now handled by the twobody.propagator module

@Sedictious Sedictious force-pushed the Sedictious:CZLM branch from c58437b to 90d6b95 Mar 27, 2019

@Juanlu001 Juanlu001 merged commit c8f316c into poliastro:master Mar 27, 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 Approved by Juan Luis Cano Rodríguez.
Details
codecov/patch Coverage not affected when comparing a8c30b7...90d6b95
Details
codecov/project 84.87% remains the same compared to a8c30b7
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Mar 27, 2019

Merged! Congratulations @Sedictious for this contribution 👏 and hope it's only the beginning! 😉

@Juanlu001 Juanlu001 referenced this pull request Mar 27, 2019

Closed

Export to CZML #117

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.