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 atmospheric models #738

Merged
merged 5 commits into from Aug 13, 2019

Conversation

@jorgepiloto
Copy link
Member

commented Jul 19, 2019

⚠️ DRAFT PULL REQUEST ⚠️

[ + ] Last update on 20th July 2019

This pull request adds different atmospheric models to improve results on drag perturbations, as requested in #702. It may be the solution to #694. The following atmospheric models are planned to be implemented:

  • ISA-ICAO: accurate up to 80 km.
  • COESA62: accurate up to 86 km.
  • COESA76: accurate up to 86 km.
  • Jacchia77: from 125km up to 700km.
  • NRLMSISE-00: up to 1000km (very used for drag perturbation)

Main features:

  • The differeten atmospheric models implemented till now are instances of the Atmosphere() class. It only needs the so-called geopotential layer properties to compute all the different properties.
from poliastro.atmosphere.models import coesa62, coesa76
T, p, rho = coesa62.properties(50 * u.km)
  • Some constants on the papers such us R_earth or g0 are not updated. Others that are required are not present in poliastro.contants such us M0_air or R_air.

  • The test_perturbations[drag] should be updated in order to check if atmospheric models work properly.

  • Tests still need to be implemented.

By opening this, you can review better the code and review it as I continue adding more models 👍

@jorgepiloto jorgepiloto reopened this Jul 19, 2019

@jorgepiloto

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2019

Sorry, I forgot about the option Draft Pull Request 😓

@codecov

This comment has been minimized.

Copy link

commented Jul 19, 2019

Codecov Report

Merging #738 into master will decrease coverage by 1.39%.
The diff coverage is 59.74%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #738     +/-   ##
=========================================
- Coverage   88.88%   87.49%   -1.4%     
=========================================
  Files          53       55      +2     
  Lines        3068     3222    +154     
  Branches      257      279     +22     
=========================================
+ Hits         2727     2819     +92     
- Misses        273      327     +54     
- Partials       68       76      +8
Impacted Files Coverage Δ
src/poliastro/atmosphere/util.py 100% <100%> (ø)
src/poliastro/atmosphere/models.py 56.02% <56.02%> (ø)

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 83d31e5...c63168c. Read the comment docs.

@Juanlu001

This comment has been minimized.

Copy link
Member

commented Jul 20, 2019

Gave a quick look, no major issues found :) Just left one comment.

On the other hand, even though I agree with you that it's better to not use classes when not necessary, I think we will encounter the need to pass "a generic atmosphere" around, or make it a configurable thing. In that case, it's easier to pass an object than a module, I think. Either of those is a form of namespacing, but objects are easier to inspect.

@jorgepiloto

This comment has been minimized.

Copy link
Member Author

commented Jul 20, 2019

Moving to classes after not only reading the previous comment, but also seeing that most atmospheric models are defined by layers and methods are shared among them. 😅

@jorgepiloto jorgepiloto force-pushed the jorgepiloto:atmosphere branch from 2646bd9 to fcfa98d Jul 20, 2019

@Juanlu001

This comment has been minimized.

Copy link
Member

commented Jul 21, 2019

In case serves you as inspiration:

We can either get inspiration from them, or include them as dependencies, or ignore them (probably a bad take!)

@jorgepiloto jorgepiloto force-pushed the jorgepiloto:atmosphere branch 3 times, most recently from 11269c2 to 4ee31e7 Jul 22, 2019

@jorgepiloto

This comment has been minimized.

Copy link
Member Author

commented Jul 25, 2019

The COESA76 works up to 86km for any variable and only up to 1000km for the temperature. Regarding COESA62 I still need to copy the layer parameters from the tables but the mathematics are exactly the same ones behind coesa76 and even easier.

@jorgepiloto jorgepiloto force-pushed the jorgepiloto:atmosphere branch from 4ee31e7 to 95270b0 Jul 26, 2019

@Juanlu001

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

Loved the notebook 😄 Could you please rebase this on top of current master?

Also, from our private conversation:

From 86 kilometers upwards, we have to apply a long formula to obtain n_i [Equation 33c]. However one of the variables in the formula depends on that same parameter, it's very strange.

From the paper:

Neither n_i, the number densities of individual species, nor sum n_i, the sum of the individual number densities, is known directly. Consequently, pressures above 86 km cannot be computed without first determining n_i for each of the significant gas species.

And then section 1.3.2 spans for more than two pages explaining how to do such a thing. Honestly I didn't bother reading it entirely, and it's probably a little bit ambitious for this PR. Therefore, can we throw a NotImplementedError for these cases, and leave it for later?

@Juanlu001

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

@jorgepiloto please rebase!

@jorgepiloto jorgepiloto force-pushed the jorgepiloto:atmosphere branch from 95270b0 to 92b6509 Aug 7, 2019

@jorgepiloto

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2019

Sure! Hope to add all requested changes in a couple of hours 🚀

@jorgepiloto

This comment has been minimized.

Copy link
Member Author

commented Aug 7, 2019

I am adding some tests, hope to be ready soon 👍

@Juanlu001

This comment has been minimized.

Copy link
Member

commented Aug 7, 2019

sphinx.errors.SphinxWarning: autodoc: failed to import module 'modules' from module 'poliastro.atmosphere'; the following exception was raised:
No module named 'poliastro.atmosphere.modules'

@jorgepiloto jorgepiloto force-pushed the jorgepiloto:atmosphere branch 2 times, most recently from ddd5f93 to ba08cdb Aug 9, 2019

@Juanlu001
Copy link
Member

left a comment

Left some very minor comments, but apart from that this is good to go!

src/poliastro/atmosphere/models.py Show resolved Hide resolved
src/poliastro/atmosphere/models.py Outdated Show resolved Hide resolved
src/poliastro/atmosphere/models.py Outdated Show resolved Hide resolved
src/poliastro/atmosphere/models.py Outdated Show resolved Hide resolved
src/poliastro/atmosphere/util.py Outdated Show resolved Hide resolved

@jorgepiloto jorgepiloto force-pushed the jorgepiloto:atmosphere branch from ba08cdb to c63168c Aug 12, 2019

@Juanlu001
Copy link
Member

left a comment

Excellent work @jorgepiloto 💪

@Juanlu001 Juanlu001 merged commit e858999 into poliastro:master Aug 13, 2019

6 of 9 checks passed

codeclimate 10 issues to fix
Details
codecov/patch 59.74% of diff hit (target 88.88%)
Details
codecov/project 87.49% (-1.4%) compared to 2c4355e
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_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
@jorgepiloto jorgepiloto referenced this pull request Aug 14, 2019
2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.