-
Notifications
You must be signed in to change notification settings - Fork 34
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
The pull request for the part of gravity anomaly from SUSTech #12
Conversation
👋 Hi @thast would you be willing to review the notebook added by @wwzzyyzzrr? |
on my to-do list. |
many thanks @thast :) Please let me know if you need anything from me |
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. |
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:
- SimPEG.PF have been fixed. Your PR is failing on the style test (see
the contributing page
<https://github.com/geoscixyz/geosci-labs/blob/master/CONTRIBUTING.md>
for more details). Having consistent formatting help the readability. I
will start the review once Travis passed. If you experience difficulties
with it, please feel free to contact us and we will help.
- I notice there is no comment at all in your code. Please add some
documentation to your functions and code. This will help both the review
and the maintenance, as this is likely to be used, improved and built upon
in the future.
- Once the PR is merged, we also usually release PR 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.). If you could provide it,
that will also help both the review and the users.
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
<#12 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AWez5qzVoCjKgeVpRMfGW0_VnWGD2EjSks5vMM4ggaJpZM4avZlL>
.
|
@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. |
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:
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
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 |
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. |
Thanks, @thast : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @wwzzyyzzrr, thanks for all of your work - this is looking good! I have a few minor comments in the code and a couple notes about the notebooks. Please let me know if you have any questions:
-
In the notebooks, we prefer not to import
*
as it can make debugging challengingfrom geoscilabs.gravity.gravitySphere import *
instead, could you please explicitly import the names of the functions / classes that you need e.g.:
from geoscilabs.gravity.gravitySphere import interact_gravity_sphere, printResults
-
I really like the images showing the setup! Rather than embedding them as binary-blobs, could you please add them to the
images
directory? This makes it easier to maintain the notebooks and makes it easier to maintain them down the road. You can reference them locally right now, e.g.![gravity-setup](../images/gravity/setup)
environment.yml
Outdated
@@ -17,4 +17,4 @@ dependencies: | |||
- deepdish | |||
- SimPEG | |||
- empymod | |||
- git+https://github.com/geoscixyz/geosci-labs | |||
- git+https://github.com/wwzzyyzzrr/geosci-labs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you please revert this change? the environment.yml should point to the main repository.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the future, you can just create a branch on this repo rather than using a fork and that should simplify things a bit. You should now have the ability to push to this repo since you are a geoscixyz team member 🎉
geoscilabs/gravity/gravityDike.py
Outdated
) | ||
|
||
|
||
if __name__ == "__main__": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this doesn't need to be here. It is really only necessary if you expect people to try and run this file as a python script, but users will be interacting with it by importing the package.
requirements.txt
Outdated
@@ -9,4 +9,4 @@ ipywidgets | |||
deepdish | |||
Pillow | |||
requests | |||
git+https://github.com/geoscixyz/geoscilabs | |||
git+https://github.com/wwzzyyzzrr/geosci-labs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment here, please use the geoscixyz version, not the fork
setup.py
Outdated
@@ -51,7 +51,7 @@ | |||
long_description=LONG_DESCRIPTION, | |||
keywords="geophysics, electromagnetics", | |||
url="https://geosci.xyz", | |||
download_url="https://github.com/geoscixyz/geoscilabs", | |||
download_url="https://github.com/wwzzyyzzrr/geosci-labs", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and again here. Please stick with the geoscixyz version
Hi, @lheagy : |
geoscilabs/gravity/gravityDike.py
Outdated
|
||
# get the value of the delta gravity | ||
def calculategravity(delta_rho, z1, z2, b, beta, x) -> float: | ||
G = 6.67259e-8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better here to add in the import:
from scipy.constants import G
(be careful with the units)
@@ -51,7 +51,7 @@ | |||
long_description=LONG_DESCRIPTION, | |||
keywords="geophysics, electromagnetics", | |||
url="https://geosci.xyz", | |||
download_url="https://github.com/geoscixyz/geoscilabs", | |||
download_url="https://github.com/geoscixyz/geosci-labs", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch!
currentResult = [] | ||
|
||
# The main function and draw the first table | ||
def drawfunction(delta_rho, z1, z2, b, beta, stationSpacing, B): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to have documentation such as:
def forward(nD,delta_rho):
"""
Parameters
----------
nD: int
number of data
delta_rho : numpy.array
model of density contrasts
Returns
-------
np.array
data, the forward data
"""
But maybe not required for this PR. What's your opinion on it @lheagy ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice and very much appreciated, but I don't think we can ask that it be required for this PR. We haven't set that precedent within the codebase yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good!
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for leading the review @thast and for all of your work on this @wwzzyyzzrr! If you can address @thast's comments then it is good-to-go on my end.
@thast: are you willing to do the release?
- merge to master
- pull master, run
coda minor
(since there is new functionality), push - generate release notes and check that once travis has run, the new version of
geoscilabs
is on pypi (https://pypi.org/project/geoscilabs/)
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.