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

Extension of COESA model to 86km - 2nd try #17

Merged
merged 3 commits into from May 26, 2014

Conversation

Gillu13
Copy link
Contributor

@Gillu13 Gillu13 commented May 25, 2014

Hello Juan,

Here is a new pull request for the extension of the COESA model.

I got rid of the unnecessary np.array() by changing them for np.asarray() which is indeed much faster.
I used np.exp rather than scipy.exp thus avoiding an import.
I PEP8-cleaned coesa.py even though it is not perfect yet.
I did not create a constant module since most of the constants are either very specific to coesa or already available in the scipy.constants module. It's up to you if you want to create such a module, maybe it can be useful when this scikit grows.

I think this is better now. Please, do not hesitate if you have further remarks.

Finally, I followed the steps you kindly suggested to me regarding git. Everything went all right as you can see. Thank you.

Regards.

Gilles

import numpy as np

# effective earth's radius
R_Earth = 6356.7660*10**3 # m
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better use the scientific notation! :) 6356.7660e3

@astrojuanlu
Copy link
Member

Hello Guilles! I spotted some style issues but other than that I think it's perfect! Would you please add a couple of tests to ensure everything is working as expected? Nothing too complicated, you can pick five or six values of altitude up to 86 km and check for the temperature, pressure and density, as it's already done up to 11000 km. When you get it I think this is ready to merge!

@Gillu13
Copy link
Contributor Author

Gillu13 commented May 25, 2014

Thank you very much for this fast review and these suggestions. I took them into account and added two test functions in test_coesa.py. I had to split it into 2 test functions because of the resolution of pressure and, to a lesser extent, of density in the standard's table (for instance, pressure resolution is 1 Pa @ 15km and 0.00001 Pa @ 86km.

Hopefully, this is ready for merging but please do not hesitate if you have further remarks.

I pushed the changes in the coesa-extension branch on Gillu13/scikit-aero, I don't know if it automatically updated the pull-request. Let me know if you can see the changes.

Best regards.

@astrojuanlu astrojuanlu merged commit 33f515e into AeroPython:master May 26, 2014
astrojuanlu added a commit that referenced this pull request May 26, 2014
@astrojuanlu
Copy link
Member

Perfect, thanks! I manually merged the commits :) And yes, as soon as you push to the PR branch the commits get updated automatically.

Thank you very much for your contributions! Let me know if you use this library or if you have any further suggestions 😄

@astrojuanlu
Copy link
Member

One more thing @Gillu13, would you please open a pull request to add yourself to AUTHORS.rst? :) Remember to work on the master branch and to update all the changes we've done.

git checkout master
git pull https://github.com/Pybonacci/scikit-aero.git master

Then edit the file adding a brief description of your contribution (e.g. "Completed COESA model up to 86km") and open a new pull request.

@astrojuanlu
Copy link
Member

I mean AUTHORS, sorry.

@Gillu13
Copy link
Contributor Author

Gillu13 commented May 26, 2014

Wonderful ! Thank you. I will update the AUTHORS file right away !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants