serialization of basemesh #182

lheagy · 2019-07-22T22:35:35Z

update instantiation of basemesh so meshes can be formed from their serialized state, e.g.

mesh = discretize.BaseMesh.deserialize(mesh_json)

will have the value of _n stored, thus, n does not need to be provided on instantiation

…erialized state

… the x0 property in the tree mesh so that x0 is stored at the python layer and a validator calls _set_x0 which lives at the cpp layer to clear the grids and update cell locaitons

tests/tree/test_tree.py

…as well as instantiation from the serialized dictionary.

codecov · 2019-07-26T19:01:37Z

Codecov Report

Merging #182 into master will decrease coverage by 0.01%.
The diff coverage is 84.61%.

@@            Coverage Diff             @@
##           master     #182      +/-   ##
==========================================
- Coverage    82.1%   82.09%   -0.02%     
==========================================
  Files          23       23              
  Lines        4974     4982       +8     
==========================================
+ Hits         4084     4090       +6     
- Misses        890      892       +2

Impacted Files	Coverage Δ
discretize/base/base_mesh.py	`79.51% <100%> (+0.07%)`	⬆️
discretize/TreeMesh.py	`57.56% <75%> (+0.29%)`	⬆️

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 cd7c205...d1f1a74. Read the comment docs.

codecov · 2019-07-26T19:01:37Z

Codecov Report

Merging #182 into master will increase coverage by 0.06%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##           master     #182      +/-   ##
==========================================
+ Coverage    82.1%   82.17%   +0.06%     
==========================================
  Files          23       23              
  Lines        4974     4993      +19     
==========================================
+ Hits         4084     4103      +19     
  Misses        890      890

Impacted Files	Coverage Δ
discretize/base/base_tensor_mesh.py	`90.47% <ø> (ø)`	⬆️
discretize/base/base_mesh.py	`79.51% <100%> (+0.07%)`	⬆️
discretize/utils/codeutils.py	`85.71% <100%> (ø)`	⬆️
discretize/TreeMesh.py	`59.19% <90.47%> (+1.92%)`	⬆️
discretize/utils/meshutils.py	`93.75% <0%> (+0.36%)`	⬆️

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 cd7c205...497f25d. Read the comment docs.

…or mesh

…nstatiation

…elated to numpy/numpy#2543

lheagy · 2019-08-01T15:44:20Z

Got it sorted @jcapriot: It was a subtle error with the behaviour of np.int32 related to numpy/numpy#2543

…y array using isinstance rather than type

lheagy · 2019-08-01T20:50:56Z

👋 @jcapriot: whenever you have a chance, would you mind taking another look and if you think it is good to go then approve the pr? or if there are still changes you see fit, please let me know. Thanks!

jcapriot · 2019-08-02T03:49:27Z

@lheagy It looks good to me. Can you look at the TreeMesh serialize/deserialize too?

jcapriot

This looks good to me! Serialization of all the meshes!

banesullivan

I think it would be great to have save/read convenience methods so that users could easily call .save() on any mesh to serialize it to a file.

Also, it would be great to have a convenience method to load any serialized discretize mesh so that you don't have to know what kind of mesh is serialized. For example:

import discretize

mesh = discretize.read('my_serialized_mesh.msh')

That way a user doesn't have to call a class-specific deserialize method in addition to reading it from the file.

mesh = discretize.TreeMesh.deserialize(read_from_file)

Or is this type of functionality already implemented? It's not clear to me.

In summary, I think there should be a simple way to save/load any mesh so that a user doesn't have to remember if they used a TensorMesh or a TreeMesh, etc.

banesullivan · 2019-08-06T21:38:06Z

discretize/TreeMesh.py

@@ -632,8 +649,17 @@ def plotSlice(
            plt.show()
        return tuple(out)

-    def save(self, *args, **kwargs):


Could this method stay and allow a user to pass a file name which will serialize any mesh to a file? For example:

mesh = some_discretize_mesh_of_any_type mesh.save('my_awesome_mesh.msh')

It was removed because It was originally overriding it here to explicitly say that save wouldn't work on the TreeMesh, now it behaves like the other meshes

The msh and mesh extensions are greatly overused across viz libraries and other mesh analysis software. I think it would be best to step away from the msh/mesh convention I've seen used with discretize and towards something more distinct.

I think this PR would be a great place to standardize how meshes are saved - perhaps have the default extension to this save method be something like: .dmesh, .discretize, .simpeg

It was removed because It was originally overriding it here to explicitly say that save wouldn't work on the TreeMesh, now it behaves like the other meshes

Ah whoops - I wasn't looking closely enough - I see now that save is indeed implemented here for all meshes

discretize/discretize/base/base_mesh.py

Line 395 in 7c0af69

def save(self, filename='mesh.json', verbose=False):

My note about the file extension remains - instead of .json, would something more distinct be appropriate?

Thanks @banesullivan: yes, the save / load should be working for all mesh-types at this point in discretize.

I am inclined to leave the file extension as json as we can then take advantage of viewers and and tools for json. For example, in JupyterLab, there is a json viewer that nests properties nicely and such, but this by default will only work if the file extension is json (e.g. I renamed the one on the right to .model).

Do you have a use-case in mind for why we would need it to be something more distinct?

lheagy · 2019-08-06T23:17:52Z

I will merge this in as discretize 0.4.9. Feel free to move the conversation about file extensions for serialized meshes to new a issue @banesullivan - happy to continue the brainstorming 😄

prisae · 2020-05-11T13:44:23Z

discretize/utils/codeutils.py

        return True
-    elif isinstance(f, np.ndarray) and f.size == 1 and type(f[0]) in scalarTypes:
+    elif isinstance(f, np.ndarray) and f.size == 1 and isinstance(f[0], scalarTypes):
        return True
    return False



Sorry for commenting on an old PR @lheagy. I just stumbled upon that and I was wondering if there is any benefit of discretize.utils.codeutils.isScalar(f) (lines 6-15) over simply np.size(f) == 1? Just thought because you extended it for complex number, so it is fragile on the defined scalarTypes and will fail for all others (e.g., int8, float32, complex64, etc).

No worries @prisae, thanks for raising this. In the scalarTypes we do have np.number which np.int8 and others inherit.

I think that just checking size isn't quite enough. For example, a 2D 1-element array currently returns False from isScalar as the first entry is an array. I think that that logic is what we want.

For example in the TensorType code, we should never be working with 2D arrays.

https://github.com/simpeg/discretize/blob/master/discretize/utils/matutils.py#L299-L309

However, this might take a bit more thought. Maybe we do want anything that has one element to evaluate True?

Yes, it might be good to think about it and agree on that, and then write a little docstring with it. Because the behaviour of isScalar is not quite obvious from just looking at it.

Here some values

np.isscalar(x) np.ndim(x) np.size(x)==1 isScalar(x)

0 True 0 True True

np.array(0) False 0 True IndexError

np.atleast_1d(0) False 1 True True

np.atleast_2d(0) False 2 True False

np.atleast_3d(0) False 3 True False

isScalar currently fails for a 0D numpy array. However, instead of fixing isScalar for that case we might consider its purpose first.

What is the use-case that a one-element 1D array passes as scalar, but a one-element 2D or 3D array not?

Here the failure for the 0D numpy array:

In [58]: isScalar(np.array(0)) --------------------------------------------------------------------------- IndexError Traceback (most recent call last) <ipython-input-58-525f9e80cd80> in <module> ----> 1 isScalar(np.array(0)) ~/anaconda3/lib/python3.7/site-packages/discretize/utils/codeutils.py in isScalar(f) 11 if isinstance(f, scalarTypes): 12 return True ---> 13 elif isinstance(f, np.ndarray) and f.size == 1 and isinstance(f[0], scalarTypes): 14 return True 15 return False IndexError: too many indices for array

lheagy added 2 commits July 22, 2019 12:51

update instantiation of basemesh so meshes can be formed from their s…

7b01348

…erialized state

add testing for forming all mesh types from a serialized mesh. update…

95288a2

… the x0 property in the tree mesh so that x0 is stored at the python layer and a validator calls _set_x0 which lives at the cpp layer to clear the grids and update cell locaitons

jcapriot reviewed Jul 24, 2019

View reviewed changes

tests/tree/test_tree.py Show resolved Hide resolved

jcapriot added 2 commits July 26, 2019 13:00

No need to guard here anymore

66a9703

Simplistic serialize and deserialize methods for the TreeMesh class, …

d1f1a74

…as well as instantiation from the serialized dictionary.

lheagy added 6 commits July 30, 2019 16:01

more informative error handling in the setting of x0 in the base tens…

ceb040a

…or mesh

update error handling in base tensor mesh instantiation

35a27e5

print the type of x_i in the error handling of the base tensor mesh i…

7ed9c4c

…nstatiation

add numpy.int32 to the scalar types in the utils.isScalar

d8fbe27

use isinstance to check types in isScalar

d608045

use np.number in the isinstance check of isScalar - I think this is r…

adc6d0b

…elated to numpy/numpy#2543

lheagy added 2 commits August 1, 2019 08:53

use numpy.number to check isScalar

9729939

allow complex values in isScalar, check the type of entries in a nump…

497f25d

…y array using isinstance rather than type

jcapriot approved these changes Aug 6, 2019

View reviewed changes

banesullivan reviewed Aug 6, 2019

View reviewed changes

lheagy merged commit ca1e578 into master Aug 6, 2019

lheagy deleted the serialization branch August 6, 2019 23:18

prisae reviewed May 11, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

serialization of basemesh #182

serialization of basemesh #182

lheagy commented Jul 22, 2019

codecov bot commented Jul 26, 2019

codecov bot commented Jul 26, 2019 •

edited

Loading

lheagy commented Aug 1, 2019

lheagy commented Aug 1, 2019

jcapriot commented Aug 2, 2019

jcapriot left a comment

banesullivan left a comment •

edited

Loading

banesullivan Aug 6, 2019 •

edited

Loading

jcapriot Aug 6, 2019

banesullivan Aug 6, 2019

banesullivan Aug 6, 2019

banesullivan Aug 6, 2019

lheagy Aug 6, 2019

lheagy commented Aug 6, 2019

prisae May 11, 2020

lheagy May 11, 2020

prisae May 12, 2020

prisae May 12, 2020

	np.isscalar(x)	np.ndim(x)	np.size(x)==1	isScalar(x)
0	True	0	True	True
np.array(0)	False	0	True	IndexError
np.atleast_1d(0)	False	1	True	True
np.atleast_2d(0)	False	2	True	False
np.atleast_3d(0)	False	3	True	False

serialization of basemesh #182

serialization of basemesh #182

Conversation

lheagy commented Jul 22, 2019

codecov bot commented Jul 26, 2019

Codecov Report

codecov bot commented Jul 26, 2019 • edited Loading

Codecov Report

lheagy commented Aug 1, 2019

lheagy commented Aug 1, 2019

jcapriot commented Aug 2, 2019

jcapriot left a comment

Choose a reason for hiding this comment

banesullivan left a comment • edited Loading

Choose a reason for hiding this comment

banesullivan Aug 6, 2019 • edited Loading

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

Choose a reason for hiding this comment

lheagy commented Aug 6, 2019

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

codecov bot commented Jul 26, 2019 •

edited

Loading

banesullivan left a comment •

edited

Loading

banesullivan Aug 6, 2019 •

edited

Loading