Add representation methods for TensorMesh #143

banesullivan · 2019-03-20T19:57:58Z

Add representation methods for `TensorMesh`

The current TensorMesh-string method is somewhat outdated, particularly for big meshes. This PR adds html and non-html representations which should be more generally applicable, for small and big TensorMesh's.

Based on work by @banesullivan on vtki and @prisae on the printversion-tool.

prisae · 2019-03-20T20:08:29Z

Great, thanks for this quick draft. Could you put a screenshot of a more complicated model, along the lines of #142 ?

discretize.TensorMesh(
    [[(25, 59, -1.03), (25, 10), (25, 59, 1.03)],
     [(50, 59, -1.02), (50, 10), (50, 59, 1.02)],
     [(20, 59, -1.05), (20, 10), (20, 59, 1.05)]],
    x0='CCC')

Also, my points from there hold:

I think minimum cell-width in each distance would be useful.

So besides X/Y/Z Bounds also min(X/Y/Z) (which is simply min(mesh.hx), max(mesh.hx)).

Also, maybe split the number of cells. Maybe leave Dimensionality out, but then Number of cells: 10 x 20 x 30, so the dimensionality can be deduced from it.

Here again the 'sketch' from #142:

discretize.TensorMesh: 128 x 128 x 128 cells; 2,097,152 in total

    dir          min/max N            min/max h
    -----------------------------------------------------
     x:      -4176.3 / 4176.3;     25.0 / 143.0
     y:      -5902.6 / 5902.6;     50.0 / 160.8
     z:      -7151.7 / 7151.7;     20.0 / 355.8

prisae · 2019-03-20T20:10:58Z

That is a really good starting point. I'll see if I get around soonish to polish it

banesullivan · 2019-03-20T20:12:30Z

discretize.TensorMesh(
    [[(25, 59, -1.03), (25, 10), (25, 59, 1.03)],
     [(50, 59, -1.02), (50, 10), (50, 59, 1.02)],
     [(20, 59, -1.05), (20, 10), (20, 59, 1.05)]],
    x0='CCC')

We could do some restructuring of the table layout that's currently there to accommodate that information.

codecov · 2019-03-20T21:20:51Z

Codecov Report

Merging #143 into master will decrease coverage by 0.46%.
The diff coverage is 58.06%.

@@            Coverage Diff             @@
##           master     #143      +/-   ##
==========================================
- Coverage   82.53%   82.07%   -0.47%     
==========================================
  Files          22       22              
  Lines        4725     4743      +18     
==========================================
- Hits         3900     3893       -7     
- Misses        825      850      +25

Impacted Files	Coverage Δ
discretize/TensorMesh.py	`80% <58.06%> (-11.45%)`	⬇️

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 1966475...f75ab55. Read the comment docs.

lheagy · 2019-03-20T22:55:06Z

Very cool! I think this would be a great improvement to discretize. I don't think the print(mesh) has seen much use, so a cleaner version that is useful for you will likely be useful to others

(also I had not seen a "draft" pr before - that is nice feature)

prisae · 2019-03-24T20:35:12Z

First go at it, inputs welcome.

@banesullivan, I simplified some stuff, because I didn't quite understand it. Maybe I simplified to much, can you have a look.

@fourndo, I don't know if that also works for TreeMeshes, could you have a go? Similar for CylMeshes @lheagy.

Just saw that I miss the tiles in the non-html version.

prisae · 2019-03-25T11:29:24Z

I improved the text-version:

prisae · 2019-03-25T11:33:13Z

@banesullivan, you put it into discretize/BaseMesh.py. I moved it now to discretize/TensorMesh.py, and replaced there the old __str__ function within the TensorMesh-class that served for printing until now.

This means it is only for TensorMeshes and not other meshes, right now. I think this is the least friction, so it should be straight forward to merge. Similar steps should be taken to replace the __str__-functions in other meshes.

prisae · 2019-03-25T12:10:56Z

prisae · 2019-03-25T12:33:53Z

The failures are related to Python 2.7. But it is an easy one to avoid, will do it.

prisae · 2019-03-25T19:56:50Z

😄 Sorry @banesullivan, messed that up with assigning. Wanted to assign you as reviewer. But I think as you originally created it it doesn't take you as reviewer...

The max stretching factor calculation failed if there was only one cell. Fixed now.

banesullivan · 2019-03-31T16:56:19Z

This looks really good @prisae! +1

Afterthoughts

This means it is only for TensorMeshes and not other meshes, right now. I think this is the least friction, so it should be straight forward to merge.

Why can't this be generalized in the BaseMesh class? Should there eventually be a generalized version of this?

A few thoughts arise on other attributes to include:

What about things like volume/area? - I'm often most concerned with the general size and extents of a mesh. Would this be something worth including?
Should there be a function that can print meta info about a scalar dictionary? discretize doesn't exactly enforce any types for data attributes to a mesh but it might be nice to have a function that can take a dictionary of arrays and print out descriptive statistics on those arrays - perhaps min, max, size, mean, std. I'm thinking this might look like pandas' .describe function. The function could be associated with the mesh and help decipher if arrays in that dictionary lie on the nodes, cells, or faces of a mesh based on their size.
- This might spark a different discussion on whether or not discretize should be able to handle pandas.DataFrames and Series

prisae · 2019-03-31T18:21:59Z

Thanks @banesullivan for the feedback!

Why can't this be generalized in the BaseMesh class? Should there eventually be a generalized version of this?

Well. There are a couple of thoughs:

The currently __str__implementation is for each mesh separate, so it is the easiest to just improve them. And I guess there is a reason why they are in the different meshes, and not in BaseMesh.
I never worked with cylindrical or OctTree meshes, so I wouldn't know what is important there. So again, I am going the easy path here.
I think different meshes have very distinct characteristics. For instance cylindrical meshes and tensor meshes are quite different, so I think it is actually best to keep this as a per-mesh basis.

So, in summary. Maybe it could be combined into a more general thing into BaseMesh. However, this would need input from various other people, and it might take some effort and time to get it done (and might get buried under other things and never done). So I preferred to do what I can and get it included. But besides that, I doubt if it makes sense to generalize it too much so it fits into BaseMesh. (Pinging here @lheagy, @rowanc1, @sgkang, @fourndo for other opinions.)

Regarding your first point in the list: I am generally interested about the extent of a mesh, and the number of cells in a mesh. The first point to see if I cover my model and my sources/receivers, the second one to know if my memory will be able to handle the calculation. So I personally am not bothered about volume nor area. But this might be different for other folks.

I think it would be good if you create a separate issue with your second point in the list.

lheagy · 2019-04-09T22:36:50Z

This is looking great! A couple questions / thoughts

the notation min(N) max(N) isn't immediately evident to me, the N is for node? Is there space to spell out node instead of the N or does that get cluttered?
It might be nice to include the origin as a column?
Similar to @prisae's comment: I haven't come across many cases where the volume or area of the entire mesh is a particularly insightful quantity
For cylindrical meshes: they aren't particularly different than Tensor Meshes (the Cylindrical Mesh class inherits the TensorMesh class). For the information shown in these print-outs, I think the print-out for the Cyl mesh could be identical to the Tensor
The OcTree Mesh is quite different, here are a few parameters I think might be useful to show. @jcapriot, @fourndo might have a few other ideas to share.
- number of total cells
- number of levels of refinement
- smallest cell size along each dimension
- largest cell size along each dimension
- origin

lheagy

This is looking great! I would be happy to merge this as is and we can iterate in new prs. Thoughts @banesullivan, @prisae?

lheagy · 2019-04-09T22:39:31Z

discretize/TensorMesh.py

+    def _get_attrs(self):
+        """An internal helper for the representation methods"""
+        attrs = []
+        dims = ['x', 'y', 'z']


this could be a property on the mesh - it would make it easier to inherit (then on the cyl mesh for example, all of this code could be re-used, but dims = ['r', 'theta', 'z']

prisae · 2019-04-10T06:18:39Z

the notation min(N) max(N) isn't immediately evident to me, the N is for node? Is there space to spell out node instead of the N or does that get cluttered?

Yes, N was meant for node. I think min(node) and max(node) would fit without any issues. Think about it, I thought simply min, max might be sufficient and even clearer? It is simply min/max of the dimension.

It might be nice to include the origin as a column?

Will do, good input!

prisae · 2019-04-10T06:58:18Z

It might be nice to include the origin as a column?

Thinking about it: What do you mean with origin? that is basically what min(N) did so far, right?

prisae · 2019-04-10T07:17:04Z

Have a look at the new screenshot. I changed min(N) to origin, and max(N) to extent. Is it clearer like that, or any other suggestions?

Also, I converted _get_attrs into a property attributes, and instead of a tuple a made a dict, so that the content is properly named. Also, I added the property dim_names, this is what would change then in cylmesh, correct? Please have a look @lheagy and let me know.

prisae · 2019-04-10T07:26:51Z

The alternative to a attributes-dict would be to create directly parameters along the line of x0:

self.x1 = (extent x, extent y, extent z)
self.h0 = (min(hx), min(hy), min(hz))
self.h1 = (max(hx), max(hy), max(hz))
self.f0 = (min factor x, min factor y, min factor z)
self.f1 = (max factor x, max factor y, max factor z)

Opinions?

lheagy

This is looking good! My main comment is that I am hesitant to add a somewhat generic property attributes to the tensor mesh. The tensor mesh is somewhat over-loaded as-is (e.g. see the docs... http://discretize.simpeg.xyz/en/master/api/generated/discretize.TensorMesh.html#module-discretize.TensorMesh), and most of the info in attributes is already captured in some form through other properties.

Do you think we can make the print function still fairly transparent without it? or maybe have it as a hidden property for now?

lheagy · 2019-04-23T23:47:08Z

discretize/TensorMesh.py

+
+            # Get min/max node.
+            n_vector = getattr(self, 'vectorN'+name)
+            attrs[name]['origin'] = np.nanmin(n_vector)


The origin of the mesh is also mesh.x0. I am wondering if that might be a bit more transparent here?

lheagy · 2019-04-23T23:59:31Z

discretize/TensorMesh.py

+            fmt += "<tr>"  # Start row
+            fmt += "<td>{}</td>".format(name)
+            fmt += "<td>{}</td>".format(iattr['nC'])
+            for p in ['origin', 'extent', 'h_min', 'h_max', 'max_fact']:


Rather than showing the extent, I think min and max is easier to interpret. For example, if you move the origin, the extent will change (e.g. origin at zero

origin at zero

arbitrary origin

I realize that the min is redundant with the origin, so we could possibly remove that.

Yes, let's agree on naming before I make any further changes: At the beginning I had min(N)/max(N). You thought that is not very intuitive, which I agree, and we should add the origin. But the origin is the same as the minimum. So I changed min(N)/max(N) to origin/extent.

I think the possibilities are:

min, start, origin, x0

max, end, extent

Having in mind that there is also min(h)/max(h) and we should avoid confusion.

Another idea might be to have a double-line header:

== TensorMesh: 2,097,152 cells == mesh extent cell width str fact dir nC min max min max max -------------------------------------------------------------------- x 128 0.00 8,352.67 25.00 143.00 1.03 y 128 -5,902.58 5,902.58 50.00 160.83 1.02 z 128 -14,303.35 0.00 20.00 355.79 1.05

I quite like the double-line header example you have listed last. It displays a lot of useful info, and I think is fairly clear to interpret.

lheagy · 2019-04-24T00:03:27Z

discretize/TensorMesh.py

+        return ['x', 'y', 'z'][:self.dim]
+
+    @property
+    def attributes(self):


I appreciate that this is handy, but I am a bit hesitant at this point to add another property to the Tensor Mesh that contains much of the same information as is already available through other properties (e.g. the list in the docs is pretty extensive already... http://discretize.simpeg.xyz/en/master/api/generated/discretize.TensorMesh.html#module-discretize.TensorMesh)

What do you think of performing these actions in the print statement?

@lheagy, you confuse me here... 😄

I made this a property because in your previous review (#143 (review)) you said "this could be a property on the mesh - it would make it easier to inherit (then on the cyl mesh for example, all of this code could be re-used". But I am happy to go back to what it was.

Sorry for the confusion! In the previous review, I was only referring to the dimension_names, and it was really only after working on #134 that I got a sense of how over-loaded the attributes are.

If you prefer to leave this as a property, I think it is okay, as long as we make it a private variable e.g. _attributes

prisae · 2019-04-26T08:13:35Z

I made a double header line in both, plain text and html. Also, the attributes is private again, _repr_attributes. I used quite some additional vertical space (more lines) in the html-implementation, but I think it is clearer and easier to read this way.

lheagy

This looks great - many thanks @prisae 🎉

Add representation methods per #142

cd4390e

banesullivan assigned prisae Mar 20, 2019

banesullivan mentioned this pull request Mar 20, 2019

Improve print(mesh) #142

Closed

First go at it

58e088c

Move to TensorMesh, improve txt-vers

2c92b82

prisae marked this pull request as ready for review March 25, 2019 11:37

,-sep; make (non-)html more alike

1143bbe

Till python 3.4, *arg had to be last args

d3be0bc

prisae mentioned this pull request Mar 25, 2019

Add fig-parameter to plot_3d_slicer #145

Merged

prisae changed the title ~~Add representation methods per #142~~ Add representation methods for TensorMesh Mar 25, 2019

prisae requested a review from lheagy March 25, 2019 19:55

prisae assigned banesullivan and unassigned banesullivan Mar 25, 2019

Bug fix for edge case

1ccf9f5

The max stretching factor calculation failed if there was only one cell. Fixed now.

banesullivan requested review from rowanc1 and removed request for rowanc1 March 31, 2019 05:12

lheagy approved these changes Apr 9, 2019

View reviewed changes

lheagy reviewed Apr 9, 2019

View reviewed changes

Make attributes a proper property

38c771e

Match names

fc2fa99

Merge branch 'master' into repr

7f2b9bc

lheagy reviewed Apr 24, 2019

View reviewed changes

Simplification and double header line

f75ab55

lheagy approved these changes Apr 26, 2019

View reviewed changes

lheagy merged commit 30e2cb0 into master Apr 26, 2019

lheagy deleted the repr branch April 26, 2019 23:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add representation methods for TensorMesh #143

Add representation methods for TensorMesh #143

banesullivan commented Mar 20, 2019 •

edited by prisae

Loading

prisae commented Mar 20, 2019

prisae commented Mar 20, 2019

banesullivan commented Mar 20, 2019

codecov bot commented Mar 20, 2019 •

edited

Loading

lheagy commented Mar 20, 2019

prisae commented Mar 24, 2019

prisae commented Mar 25, 2019

prisae commented Mar 25, 2019

prisae commented Mar 25, 2019

prisae commented Mar 25, 2019

prisae commented Mar 25, 2019

banesullivan commented Mar 31, 2019 •

edited

Loading

prisae commented Mar 31, 2019

lheagy commented Apr 9, 2019

lheagy left a comment

lheagy Apr 9, 2019

prisae commented Apr 10, 2019 •

edited

Loading

prisae commented Apr 10, 2019

prisae commented Apr 10, 2019

prisae commented Apr 10, 2019

lheagy left a comment •

edited

Loading

lheagy Apr 23, 2019

lheagy Apr 23, 2019

prisae Apr 24, 2019

lheagy Apr 24, 2019

lheagy Apr 24, 2019

prisae Apr 24, 2019

lheagy Apr 24, 2019

prisae commented Apr 26, 2019

lheagy left a comment

Add representation methods for TensorMesh #143

Add representation methods for TensorMesh #143

Conversation

banesullivan commented Mar 20, 2019 • edited by prisae Loading

Add representation methods for TensorMesh

prisae commented Mar 20, 2019

prisae commented Mar 20, 2019

banesullivan commented Mar 20, 2019

codecov bot commented Mar 20, 2019 • edited Loading

Codecov Report

lheagy commented Mar 20, 2019

prisae commented Mar 24, 2019

prisae commented Mar 25, 2019

prisae commented Mar 25, 2019

prisae commented Mar 25, 2019

prisae commented Mar 25, 2019

prisae commented Mar 25, 2019

banesullivan commented Mar 31, 2019 • edited Loading

Afterthoughts

prisae commented Mar 31, 2019

lheagy commented Apr 9, 2019

lheagy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prisae commented Apr 10, 2019 • edited Loading

prisae commented Apr 10, 2019

prisae commented Apr 10, 2019

prisae commented Apr 10, 2019

lheagy left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

origin at zero

arbitrary origin

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prisae commented Apr 26, 2019

lheagy left a comment

Choose a reason for hiding this comment

banesullivan commented Mar 20, 2019 •

edited by prisae

Loading

Add representation methods for `TensorMesh`

codecov bot commented Mar 20, 2019 •

edited

Loading

banesullivan commented Mar 31, 2019 •

edited

Loading

prisae commented Apr 10, 2019 •

edited

Loading

lheagy left a comment •

edited

Loading