search

geoscixyz / geosci-labs

collection of notebook-apps for concepts in applied geophysics
https://geosci.xyz
MIT License
50 stars 34 forks source link

The pull request for the part of gravity anomaly from SUSTech #12

Closed MetaronWang closed 5 years ago

MetaronWang commented 5 years ago

This is Southern University of Science and Technology. We design and write a new part for this project about the gravity anomaly. We write the python file by the structure like the file in the project before, and the folder structure is also like the project structure.

lheagy commented 5 years ago

👋 Hi @thast would you be willing to review the notebook added by @wwzzyyzzrr?

thast commented 5 years ago

on my to-do list.

lheagy commented 5 years ago

many thanks @thast :) Please let me know if you need anything from me

thast commented 5 years ago

Hi @wwzzyyzzrr,

Thanks a lot for your first-time contribution to this repository, that's awesome! Gravity is a really key feature and many people will have use for it.

I want first to highlight a couple of points here before doing any in-depth review. This will help get things right from the start and the long-term use of the work you have developed:

For all those points, please take a look at the contributing page for more details (like on PEP8, Black, Flake8 for formatting, PR description etc.)

Please feel free to contact us and ask any question you might have.

@lheagy : let us know if you have any additional comment or thought about it.

MetaronWang commented 5 years ago

Hi, @Thibaut Astic: Thankyou for your work. We will finsh it in time and pull request again. And for the comments, we had found that the codes in the folder geoscilabs have no comment, so we delete all of them, what should we do for it next step? Add comments in the python file or write in document?

2019年2月11日 09:57，"Thibaut Astic" notifications@github.com写道：

Hi @wwzzyyzzrr https://github.com/wwzzyyzzrr,

Thanks a lot for your first-time contribution to this repository, that's awesome! Gravity is a really key feature and many people will have use for it.

I want first to highlight a couple of points here before doing any in-depth review. This will help get things right from the start and the long-term use of the work you have developed:

For all those points, please take a look at the contributing page https://github.com/geoscixyz/geosci-labs/blob/master/CONTRIBUTING.md for more details (like on PEP8, Black, Flake8 for formatting, PR description etc.)

Please feel free to contact us and ask any question you might have.

@lheagy https://github.com/lheagy : let us know if you have any additional comment or thought about it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/geoscixyz/geosci-labs/pull/12#issuecomment-462203018, or mute the thread https://github.com/notifications/unsubscribe-auth/AWez5qzVoCjKgeVpRMfGW0_VnWGD2EjSks5vMM4ggaJpZM4avZlL .

thast commented 5 years ago

@wwzzyyzzrr : Ah I see! Sorry for the confusion. Documentation and comments in the python file is better in my opinion, makes it easier to understand, review, debug and improve. I do not know how easy it is for you to "put it back" but that would be really valuable. Also from a long-term perspective, we would likely build the API documentation for the code comments itself.

I think that the codes that are still in geosci-labs were developed years ago, before it became a shared resources, thus a lack of time dedicated to document it. The most advanced programs got moved to SimPEG and documented, but the ones really more specific to the original teaching material stayed poorly documented.

lheagy commented 5 years ago

Many thanks @thast for leading the review! Just one minor thing I will jump in with, the test that is failing is the "style" test, meaning that the code is not formatted. From the main directory (geosci-labs) you can run:

make format

to format the code (or have a look at the Makefile and copy the command into your terminal). This runs black so that the code if formatted consistently. You can then commit and push any changes it suggests.

After this, try running

make check

and this will check that the code has been formatted and will also check that the code style (loosley) follows the pep8 guidelines for naming and a few other best-practices. If make check runs cleanly, then the test should pass on travis.

MetaronWang commented 5 years ago

I have modified the files that I think need to be changed. And the test of the flake8 is also pass, so could you review the commits of us again? Thank you.

thast commented 5 years ago

Good job @wwzzyyzzrr on the style repair :) It seems the gravity notebooks are not tested in Travis (one travis test per module, and I do not see gravity in the list). Could you add the test? Let us know if you need help. image

MetaronWang commented 5 years ago

Thanks, @thast : We have added the test file for the gravity and add it into the .travis.yml, all the tests have been passed.

MetaronWang commented 5 years ago

Hi, @lheagy : Thanks for your suggestions, I have revised the problems mentioned above.

thast commented 5 years ago

Hi @wwzzyyzzrr , thanks for your work. I have left a few comments above for you to review. If @lheagy also agree to let the documentation of the functions as it is, then switching the hard coded value of G for the import scipy.constants import G (with proper unit conversion) will do for the current PR. Please also provide releases notes: basically a description of what have been added, fixed or improved upon (like names of the new notebooks, what do they do and how etc.), to accompany the official release. It does not need to be long, just a few lines will do. Good job!