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

Volume average #212

Merged
merged 18 commits into from
Sep 2, 2020
Merged

Volume average #212

merged 18 commits into from
Sep 2, 2020

Conversation

jcapriot
Copy link
Member

This PR implements the volume averaging operation for TensorMesh and TreeMesh, as well as combinations of the two. I have used the Tensor to Tensor myself, however the Tree to X operations could use some more test driving.

Of course there's some simple tests in here to ensure it's doing what I think it should be doing.

I was also running into issues getting the numpydoc's plot directive to automatically work in the Examples section of docstrings, and the solution was to remove the napoleon docs, since we're already using the numpydoc extension, and I couldn't find any examples of Google style docstrings, I just removed that from conf.py. So if this is okay, I'd like to just remove the napoleon Sphinx extension.

@codecov
Copy link

codecov bot commented Jul 13, 2020

Codecov Report

Merging #212 into master will increase coverage by 0.09%.
The diff coverage is 96.87%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #212      +/-   ##
==========================================
+ Coverage   81.39%   81.48%   +0.09%     
==========================================
  Files          24       24              
  Lines        5197     5228      +31     
==========================================
+ Hits         4230     4260      +30     
- Misses        967      968       +1     
Impacted Files Coverage Δ
discretize/utils/meshutils.py 92.40% <ø> (ø)
discretize/utils/interputils.py 88.23% <96.77%> (+7.15%) ⬆️
discretize/utils/__init__.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a4ead91...36b6bce. Read the comment docs.

@jcapriot
Copy link
Member Author

@prisae I know I never responded to it, but I did see your comment before, and implemented something that does the same thing, but bypasses the numpy boolean indexing. Let me know if that is sufficient. The idea was to speed it up when you're interpolating a large mesh extent onto a smaller mesh extent, without loosing too much time.

@jcapriot jcapriot requested review from fourndo and lheagy July 14, 2020 20:45
@prisae
Copy link
Member

prisae commented Jul 15, 2020

@prisae I know I never responded to it, but I did see your comment before, and implemented something that does the same thing, but bypasses the numpy boolean indexing. Let me know if that is sufficient. The idea was to speed it up when you're interpolating a large mesh extent onto a smaller mesh extent, without loosing too much time.

I'll try to have a look before the meeting today.

@prisae
Copy link
Member

prisae commented Jul 15, 2020

Yes, I like it now, great work @jcapriot!

prisae added a commit to emsig/emg3d that referenced this pull request Jul 15, 2020
Improves speed if the new mesh is much smaller than the old mesh.

See simpeg/discretize#212
prisae added a commit to emsig/emg3d that referenced this pull request Jul 15, 2020
Improves speed if the new mesh is much smaller than the old mesh.

See simpeg/discretize#212
@prisae prisae linked an issue Jul 15, 2020 that may be closed by this pull request
@domfournier domfournier self-requested a review July 21, 2020 17:37
Copy link
Contributor

@domfournier domfournier left a comment

Choose a reason for hiding this comment

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

Great to have this function built-in.
So I ran a small test (notebook attached) comparing the projection matrix between this one and the one used in simpeg.maps.TileMap. The good news is that both give the same answer, even after applying active cells. This one appears to be a lot slower, however, by about a factor 10x fairly consistently.
I am guessing there is a cost for having more general Tensor <-> Octree?

Notebook

image

@jcapriot
Copy link
Member Author

jcapriot commented Jul 21, 2020

Yes, there is certainly a cost associated with the general Tensor to OcTree... but you do bring up a good point. There's no reason the function could not recognize that a Tensor and OcTree share the same base mesh and switch to the faster version.

Also, which operation is the first timing operation doing there, where the discretize one is faster than the SimPEG one?

@domfournier
Copy link
Contributor

YEa not sure why the first in the loop is different. Maybe some imports on the simpeg side?

@lheagy
Copy link
Member

lheagy commented Aug 26, 2020

@jcapriot: would it be possible to do a beta release to make this available on conda-forge? I am having trouble installing discretize from source on a remote machine at the moment...

Copy link
Member

@lheagy lheagy left a comment

Choose a reason for hiding this comment

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

This is a great addition @jcapriot! I have been testing it out on the EM problem, and it is working well for this case :)

@@ -82,3 +83,109 @@ def interpmat(locs, x, y=None, z=None):
Q = sp.csr_matrix((vals, (I, J)),
shape=(npts, np.prod(shape)))
return Q

def volume_average(mesh_in, mesh_out, values=None, output=None):
"""Volume averaging interpolation between meshes.
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for such a beautiful docstring :)

@jcapriot
Copy link
Member Author

jcapriot commented Aug 31, 2020

@domfournier Can you check the timings again for me? I added shortcuts for when meshes share the same base mesh

edited: actually never mind I'll run your notebook, thanks for that

@jcapriot jcapriot merged commit 9fcc28b into master Sep 2, 2020
@jcapriot jcapriot deleted the volume_average branch August 19, 2021 19:37
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.

Volume averaging interpolation
4 participants