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 equivalent layer for harmonic functions #78

Merged
merged 60 commits into from Sep 20, 2019

Conversation

santisoler
Copy link
Member

@santisoler santisoler commented Jul 22, 2019

Add an Equivalent Layer gridder for harmonic functions, based on
verde.BaseGridder using 1/r as the basis function. By default it puts
one point source bellow each observation point at a constant relative
depth and fits the corresponding coefficient to each source. Also
a custom set of point sources can be passed. Add a gallery example
showing how to grid a small region of the magnetic anomaly data from Rio
de Janeiro.

Fixes #39

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.
  • Write detailed docstrings for all functions/methods.
  • If adding new functionality, add an example to the docstring, gallery, and/or tutorials.

It will make easy to find future multiple EQL gridders by tab
autocompletion.
Replace the distance_to_nearest function with verde.meadian_distance and
add k_nearest as optional argument for the gridder.
This new test function is intended to be run with Numba jit disabled.
A great number of masses will heavily increase the computation time if
Numba jit is disabled.
@santisoler
Copy link
Member Author

I want to use some functions from #71 when predicting interpolated values and computing distances in Cartesian coordinates. They must be incorporated after #71 is merged.
I'm still using the term "vertical" for specifying the downward coordinates. It should be changed in agreement with the term used in #71.

@santisoler
Copy link
Member Author

It also uses some functions from the dev version of Verde. A new Verde release should be made in order to CIs to pass.

Because the vertical coordinate points downwards, the vertical component
for the point sources must be increased if we want them to be bellow the
observation points.
Co-Authored-By: Leonardo Uieda <leouieda@gmail.com>
@leouieda
Copy link
Member

@santisoler I've been experimenting with the example and code a bit.

I isolated another anomaly that was better for visualizing. This is the result using the same parameters you were using but with a diverging colormap with 0 centered on white:

meh

Notice that there are a lot of artifacts from the flight lines. The solution is also very sensitive to the depth_factor and k_nearest arguments.

These are the results with a larger grid spacing (400m) and a uniform depth (500m below data):

meh

Notice that the stripes are gone. This due mainly to the larger spacing. The single depth is also easier to understand. I think the variable depth might be useful in other circumstances but probably not in the cases we're looking at right now. So it might be better to use the simpler case first and add the option of variable depth as we progress.

What do you think?

If you agree, I'll go ahead and commit my changes to the code and example.

@santisoler
Copy link
Member Author

Hi @leouieda! I really like those results and agree to keep things simple and try more complex layouts in the future. Feel free to commit your changes!

Removed the code for variable depth based on neighbor distances.
Use a different anomaly for the example and plot balancing the colormap.
Use deeper sources for smoother grid and appropriate grid spacing.
Simple train-test split instead of CV for quicker and easier to
understand example.
import numpy as np
import pyproj
import verde as vd
import harmonica as hm

Choose a reason for hiding this comment

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

I001 isort found an import in the wrong position

import pyproj
import verde as vd
import harmonica as hm

Choose a reason for hiding this comment

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

I004 isort found an unexpected blank line in imports


# Plot gridded magnetic anomaly
tmp = grid.magnetic_anomaly.plot.pcolormesh(ax=ax2, add_colorbar=False,
vmin=-maxabs, vmax=maxabs, cmap="seismic")

Choose a reason for hiding this comment

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

E128 continuation line under-indented for visual indent

@leouieda
Copy link
Member

@santisoler I pushed my modifications but it still needs some polish. I haven't checked the tests, the docstrings need some updating and polish, and the example plot could be nicer.

@leouieda
Copy link
Member

I'll take a look at the prism PR tomorrow.

@leouieda leouieda changed the title Add Equivalent Layer interpolator for harmonic functions Add equivalent layer for harmonic functions Sep 14, 2019
@fatiando fatiando deleted a comment from stickler-ci Sep 14, 2019
@fatiando fatiando deleted a comment from stickler-ci Sep 14, 2019
@fatiando fatiando deleted a comment from stickler-ci Sep 14, 2019
@fatiando fatiando deleted a comment from stickler-ci Sep 14, 2019
@leouieda
Copy link
Member

@santisoler this is ready for a review. Here is what I've done:

  • Placed EQL examples in their own category. This one is about gridding and upward continuation.
  • Add Dampney 1969 citation
  • Remove the variable depth code to simplify this setup
  • Rework the testing to check interpolating from one grid to a denser grid
  • Example uses another anomaly, no cross-validation (to make things simpler and because we don't know if it actually works), balances the color maps, uses a larger spacing, and more text at the top explaining what EQLs do.
  • Class docstring lists what this EQL is good for and what it can't do. This will be useful since other layers can so other things and we can link them together this way.

@leouieda
Copy link
Member

@birocoles it would be great if you took a look at this as well. Particularly the harmonica/equivalent_layer/harmonic.py and examples/eql/harmonic.py. I don't think we said anything terribly wrong but you're the real expert here 👨‍🎓.

Copy link
Member Author

@santisoler santisoler left a comment

Choose a reason for hiding this comment

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

Nice work @leouieda! This looks awesome! Both the docstring and the gallery example are very clear and I give you one point for showing both the capabilities and the flaws of the gridder.

I would make clearer what the depth argument means: at first sight it seems that all point sources are located at a constant depth by default, but in fact they are set at a constant relative depth beneath each observation point. What do you think?

@@ -92,6 +92,12 @@ jobs:
- bash: echo "##vso[task.prependpath]$CONDA/bin"
displayName: Add conda to PATH

# On Hosted macOS, the agent user doesn't have ownership of Miniconda's installation
Copy link
Member Author

Choose a reason for hiding this comment

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

Shouldn't we add these lines on a different PR?

@leouieda
Copy link
Member

I give you one point for showing both the capabilities and the flaws of the gridder.

🥇 🎊

I would make clearer what the depth argument means: at first sight it seems that all point sources are located at a constant depth by default, but in fact they are set at a constant relative depth beneath each observation point. What do you think?

I completely agree. Would you mind adding this? Feel free to merge once that's done. We can always improve the docs but gotta get it out at some point ;-)

Shouldn't we add these lines on a different PR?

You caught me being lazy... 😞 I'll open the PR.

Improve docstrings to make clearer that the relative_depth argument
represent the relative depth at which the point sources will be located
referenced to the elevation of the data points.
@santisoler
Copy link
Member Author

I renamed the depth argument to relative_depth and improved the docstrings to make it clearer what this argument represents. I've also add a little comment on the gallery example explaining this.
I'll merge this if CIs agree. I agree we can change things later.

@santisoler santisoler merged commit a618988 into fatiando:master Sep 20, 2019
@santisoler santisoler deleted the eql branch September 20, 2019 14:13
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.

General equivalent layer solution
3 participants