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

Use czml3 for the czml extractor #711

Merged
merged 15 commits into from Jun 29, 2019
Merged

Use czml3 for the czml extractor #711

merged 15 commits into from Jun 29, 2019

Conversation

@Sedictious
Copy link
Member

Sedictious commented Jun 24, 2019

This pretty much completely removes the dictionary-based czml representation of the extractor's previous implementation and uses poliastro's czml3 library instead.

(Note that there is a minor bug in the main application where the optional map needs to be defined, hence why the UV map doesn't default to none. I'll try to resolve it asap)

@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Jun 24, 2019

Please rebase on current master and fix the quality issues at your earliest convenience :) Remember you can do tox -e reformat and they will be fixed automatically.

@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Jun 24, 2019

Sorry, I spoke too fast. Leaving inline comments about mypy.

@Sedictious

This comment has been minimized.

Copy link
Member Author

Sedictious commented Jun 24, 2019

@Juanlu001 I run tox and was so ready it was going to pass circleci. Turns out a failing test is one missing type annotation away 😅

As for adding czml3 as a dependency, is there anything more I'll need to do?

Copy link
Member

Juanlu001 left a comment

Removing code feels so good :) I left some comments, mostly about not specifying the properties when they are the default values and also using more objects from czml3

src/poliastro/czml/extract_czml.py Outdated Show resolved Hide resolved
src/poliastro/czml/extract_czml.py Outdated Show resolved Hide resolved
self.czml[i]["position"]["cartesian"] += list(
map(lambda x: x[0], cords.tolist())
)
cart_cords += list(map(lambda x: x[0], cords.tolist()))

This comment has been minimized.

Copy link
@Juanlu001

Juanlu001 Jun 24, 2019

Member

Is there a more straightforward way to write this? It's a bit cryptic 😓

This comment has been minimized.

Copy link
@Sedictious

Sedictious Jun 25, 2019

Author Member

This simply flattens the list (since cords.tolist() returns a list of lists). I guess we could also use something like itertools.chain?

This comment has been minimized.

Copy link
@Juanlu001

Juanlu001 Jun 25, 2019

Member

Oh, I see! I think itertools.chain is better recognized as "flattening the list" but, whether you change it or not, please add a comment explaining

src/poliastro/czml/extract_czml.py Outdated Show resolved Hide resolved
src/poliastro/czml/extract_czml.py Show resolved Hide resolved
src/poliastro/czml/extract_czml.py Outdated Show resolved Hide resolved
src/poliastro/czml/extract_czml.py Outdated Show resolved Hide resolved
src/poliastro/czml/extract_czml.py Show resolved Hide resolved
src/poliastro/czml/extract_czml.py Outdated Show resolved Hide resolved
src/poliastro/czml/extract_czml.py Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Jun 24, 2019

As for adding czml3 as a dependency, is there anything more I'll need to do?

Good question, just left a comment for it. There should be a dedicated section for czml3 (instead of dev) but let's not worry much about that now.

Copy link
Member

Juanlu001 left a comment

Left one more minor question and we are good to go!

src/poliastro/czml/extract_czml.py Outdated Show resolved Hide resolved
src/poliastro/czml/extract_czml.py Outdated Show resolved Hide resolved
src/poliastro/czml/extract_czml.py Show resolved Hide resolved
src/poliastro/czml/extract_czml.py Show resolved Hide resolved
@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Jun 25, 2019

Some tests are failing because of... timezones 😄

E         -     "availability": "2013-03-18T12:00:00Z/2013-03-18T23:59:35.108",
E         ?                                  ^
E         +     "availability": "2013-03-18T10:00:00Z/2013-03-18T23:59:35.108",
E         ?  
@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Jun 25, 2019

Interestingly, there are also some small precision issues. This will need closer inspection.

@Juanlu001 Juanlu001 mentioned this pull request Jun 25, 2019
@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Jun 25, 2019

The test_GeocentricSolarEcliptic_against_data is failing because this branch still needs a rebase. @Sedictious when you have some spare 20 minutes ping me privately :)

@Sedictious Sedictious force-pushed the Sedictious:czml branch from 55e5ce9 to acc4e7f Jun 25, 2019
@Sedictious Sedictious force-pushed the Sedictious:czml branch from acc4e7f to e256c8d Jun 25, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Jun 28, 2019

Codecov Report

Merging #711 into master will decrease coverage by 1.34%.
The diff coverage is 19.04%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #711      +/-   ##
==========================================
- Coverage   87.44%   86.09%   -1.35%     
==========================================
  Files          53       52       -1     
  Lines        2700     2640      -60     
  Branches      241      221      -20     
==========================================
- Hits         2361     2273      -88     
- Misses        265      310      +45     
+ Partials       74       57      -17
Impacted Files Coverage Δ
src/poliastro/czml/extract_czml.py 25.64% <19.04%> (-48.05%) ⬇️
src/poliastro/czml/utils.py 28.57% <0%> (-71.43%) ⬇️

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 233a89c...24826dd. Read the comment docs.

@Sedictious Sedictious force-pushed the Sedictious:czml branch from 5360780 to b0aaed9 Jun 28, 2019
@Sedictious Sedictious force-pushed the Sedictious:czml branch from 38ae67a to 75b27d8 Jun 28, 2019
Sedictious added 2 commits Jun 29, 2019
@Juanlu001

This comment has been minimized.

Copy link
Member

Juanlu001 commented Jun 29, 2019

This PR has been in the works for far too long already, so I decided to merge it straight away and we can fix any remaining issues afterwards :) Thanks @Sedictious for the hard work!

@Juanlu001 Juanlu001 merged commit 09c469b into poliastro:master Jun 29, 2019
7 of 10 checks passed
7 of 10 checks passed
codeclimate 4 issues to fix
Details
codecov/patch 19.04% of diff hit (target 87.44%)
Details
codecov/project 86.09% (-1.35%) compared to 233a89c
Details
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
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@Juanlu001 Juanlu001 mentioned this pull request Jul 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.