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 function to create a layer of prisms #186

Merged
merged 45 commits into from
Apr 7, 2021
Merged

Add function to create a layer of prisms #186

merged 45 commits into from
Apr 7, 2021

Conversation

santisoler
Copy link
Member

@santisoler santisoler commented Sep 2, 2020

Add function that creates a layer of prisms as a xr.Dataset which contains
the coordinates of the center of each prism, its top and bottom boundaries (as
xarray coordinates) and each property (as density, magnetization, etc) as data
array. Add PrismsLayer class as a dataset accesor
for xr.Datasets, which allows to define several attributes, properties and
methods for the prism layer, which can be accessed through the prisms_layer
attribute of the xr.Dataset. Add a gravity method that computes
gravitational fields generated by every prism in the layer whose top boundary,
bottom boundary or density are not np.nans.
Add two gallery examples for the prisms layer, and a new South Africa
topography dataset, which is a downsample of ETOPO1.

This PR may serve as a prototype for solving #83, but for prisms. The one for tesseroids could be equivalent to this one, but with some minor checks due to longitude continuity.

Reminders:

  • Run make format and make check to make sure the code follows the style guide.
  • Add tests for new features or tests that would have caught the bug that you're fixing.
  • Add new public functions/methods/classes to doc/api/index.rst and the base __init__.py file for the package.
  • Write detailed docstrings for all functions/classes/methods. It often helps to design better code if you write the docstrings first.
  • If adding new functionality, add an example to the docstring, gallery, and/or tutorials.
  • Add your full name, affiliation, and ORCID (optional) to the AUTHORS.md file (if you haven't already) in case you'd like to be listed as an author on the Zenodo archive of the next release.

@santisoler santisoler changed the title WIP Add funciton to create a layer of prisms WIP Add function to create a layer of prisms Sep 21, 2020
@santisoler
Copy link
Member Author

I suspect that ETOPO1 is referenced on the geoid (need to check), so we might want to add the geoid height (from the reference ellipsoid) to the topography + bathymetry grid before creating the terrain layer.

@santisoler
Copy link
Member Author

@lperozzi Would you like to review this PR?

@santisoler santisoler changed the title WIP Add function to create a layer of prisms Add function to create a layer of prisms Oct 1, 2020
Copy link

@lperozzi lperozzi left a comment

Choose a reason for hiding this comment

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

I will add another points that I've remarked today. If the aim is to extract the topographic correction for the observations stations from the prisms gravity forward, we should be careful with the bord effects.

plt.colorbar(tmp, label="mGal")
ax.set_aspect("equal")
plt.tight_layout()
plt.show()

Choose a reason for hiding this comment

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

I do not have any specifical comments. The code is clear and easy to read, however I have a question.
Starting form a land survey, it is also possible to compute the prisms['top'] by gridding the elevation data, using verde for example? Instead to have a DEM or topographic grid for the region?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure! You'll probably want to apply some gridding or downsampling to your topographic grid in order to build the prisms: there's no point to use a very high resolution DEM to apply terrain correction, while if you have some topographic height measurements and low resolution DEM you can combine them using a gridder. But those tasks should be carried out by the user.

@santisoler
Copy link
Member Author

Thanks for the feedback @lperozzi!

I will add another points that I've remarked today.

Nice! Can't wait to see them!

@santisoler
Copy link
Member Author

If the aim is to extract the topographic correction for the observations stations from the prisms gravity forward, we should be careful with the bord effects.

I totally agree, maybe we should change the gallery example so it takes a larger region than the survey area for building the prisms. Thanks for noticing it!

@santisoler
Copy link
Member Author

santisoler commented Nov 13, 2020

Would be nice to use the new make_xarray_grid for creating the prisms layer. We could also wait for fatiando/verde#300 to be merged so we will be able to pass the easting and northing coordinates either as 1d or 2d arrays.
This way we could generate a prisms layer either from a xarray.Dataset or from numpy arrays created through the vd.grid_coordinates without any hassle like slicing 2d arrays.

@santisoler santisoler added this to the v0.2.0 milestone Feb 10, 2021
@santisoler
Copy link
Member Author

santisoler commented Mar 23, 2021

@leouieda I'm trying to finish this PR. I think it's almost ready, but I'm using the prisms_layer feature for the Transform21 tutorial and I found that it can create some problems when the surface array has nans.

I decided to be conservative and raise and error if any nan is present on surface or reference.

Another option is to have a drop_nans=False optional argument that replaces the nans on the surface for the respective value of the reference, thus generating a prisms without volume.

What do you think?

The method builds the list of prism boundaries and ignores the ones that
has nans on their top or bottom boundaries before passing them into the
prism_gravity() function.
The density of the prisms is caught from the "density" data_var present
in the xarray.Dataset by default.
@santisoler
Copy link
Member Author

@leouieda I'm trying to finish this PR. I think it's almost ready, but I'm using the prisms_layer feature for the Transform21 tutorial and I found that it can create some problems when the surface array has nans.

I decided to be conservative and raise and error if any nan is present on surface or reference.

Another option is to have a drop_nans=False optional argument that replaces the nans on the surface for the respective value of the reference, thus generating a prisms without volume.

What do you think?

We decided to allow surface array with nans and add a gravity method that computes the gravity field generated by the layer, ignoring the masked points.

Had to disable the proteceted-access on the entire test_prisms_layer.py
file, otherwise pylint will keep complaining about it, even if I disable
it function wise or even line wise.
nonans_bottom = np.logical_not(np.isnan(self._obj.bottom.values))
return np.logical_and(nonans_top, nonans_bottom)

def get_prisms(self):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def get_prisms(self):
def to_prisms(self):

This fits better with the rest of the xarray methods: http://xarray.pydata.org/en/stable/pandas.html

Copy link
Member

Choose a reason for hiding this comment

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

Could keep this private for now and provide it after if desired.

# have no nans on their top and bottom
nonans_boundaries = self._get_nonans_prisms_mask().ravel()
# Get density array
density = getattr(self._obj, density_name).values.ravel()
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
density = getattr(self._obj, density_name).values.ravel()
density = self._obj[density_name].values.ravel()

Works for density names that aren't valid Python variable names.

Write a private method that returns a mask for every prism whose top
boundary, bottom boundary or density is nan. Raise a warning if the
density array has a nan where neither the top or bottom is nan.
Rename the get_prisms() method to _to_prisms() and make it private.
Update and improve the tests.
Run the check when defining the prism layer and when computing the
spacing of the layer.
@santisoler
Copy link
Member Author

I think this is ready to go. I'll merge it even the CIs are failing: the docs build is failing because it can't download the new South Africa topography dataset. If master fails after merge, I'll fix it right away.

@santisoler santisoler merged commit 69c5a64 into master Apr 7, 2021
@santisoler santisoler deleted the prisms-layer branch April 7, 2021 20:06
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.

3 participants