Ref/properties #70

lheagy · 2017-09-01T15:50:20Z

Implement properties in discretize

Use properties to define the properties of a class that are necessary to completely define the object. This allows us to serialize, save and load meshes (also copy!)

todos

test on cyl mesh
test on tree mesh
test on curvi mesh

@rowanc1, @fwkoch: if you guys would be willing to do a review, I would appreciate another set of eyes

Related issues

private properties: properties can't be private? seequent/properties#191
arrays with arbitrary shape: default shape of properties.Array seequent/properties#192

Dev

…y, add a check to ensure the cartesian origin is being set properly. Have x0 be passed as a kwarg in meshIO

codecov · 2017-09-01T20:18:15Z

Codecov Report

Merging #70 into dev will increase coverage by <.01%.
The diff coverage is 86.55%.

@@            Coverage Diff             @@
##              dev      #70      +/-   ##
==========================================
+ Coverage   76.98%   76.98%   +<.01%     
==========================================
  Files          17       17              
  Lines        4797     4858      +61     
==========================================
+ Hits         3693     3740      +47     
- Misses       1104     1118      +14

Impacted Files	Coverage Δ
discretize/View.py	`4.91% <0%> (ø)`	⬆️
discretize/CurvilinearMesh.py	`99.4% <100%> (+0.02%)`	⬆️
discretize/MeshIO.py	`83.56% <100%> (+0.47%)`	⬆️
discretize/utils/meshutils.py	`69.35% <100%> (ø)`	⬆️
discretize/BaseMesh.py	`83.81% <77.19%> (-1.77%)`	⬇️
discretize/TensorMesh.py	`90.9% <85.71%> (-1.13%)`	⬇️
discretize/utils/__init__.py	`85.71% <85.71%> (+1.09%)`	⬆️
discretize/CylMesh.py	`84.61% <90.9%> (-0.04%)`	⬇️
discretize/TreeMesh.py	`82.8% <94.44%> (+0.04%)`	⬆️

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 4ccd933...a75afa5. Read the comment docs.

…ent/properties#191, test serialization and deserialization of tree mesh

…cussed in seequent/properties#192

lheagy · 2017-09-02T17:19:33Z

README.rst

@@ -21,9 +21,6 @@ discretize
    :target: https://www.codacy.com/app/lindseyheagy/discretize?utm_source=github.com&amp;utm_medium=referral&amp;utm_content=simpeg/discretize&amp;utm_campaign=Badge_Grade
    :alt: codacy status

-.. image:: https://www.quantifiedcode.com/api/v1/project/97126041a7e7400fbf1c5555bed514dc/badge.svg


we weren't really using this and they discontinued it

lheagy · 2017-09-02T17:20:16Z

discretize/BaseMesh.py


 class BaseRectangularMesh(BaseMesh):
    """BaseRectangularMesh"""
    def __init__(self, n, x0=None):
-        BaseMesh.__init__(self, n, x0)
+        BaseMesh.__init__(self, n, x0=x0)


this isn't strictly necessary. just for clarity

lheagy · 2017-09-02T17:20:27Z

discretize/BaseMesh.py

-        assert (type(x) == list or isinstance(x, np.ndarray)), "x must be either a list or a ndarray"
-        assert xType in ['CC', 'N', 'F', 'Fx', 'Fy', 'Fz', 'E', 'Ex', 'Ey', 'Ez'], "xType must be either 'CC', 'N', 'F', 'Fx', 'Fy', 'Fz', 'E', 'Ex', 'Ey', or 'Ez'"
-        assert outType in ['CC', 'N', 'F', 'Fx', 'Fy', 'Fz', 'E', 'Ex', 'Ey', 'Ez'], "outType must be either 'CC', 'N', 'F', Fx', 'Fy', 'Fz', 'E', 'Ex', 'Ey', or 'Ez'"
+        allowed_xType = [


whitespace cleanup

lheagy · 2017-09-02T17:21:50Z

discretize/CurvilinearMesh.py

@@ -25,7 +27,62 @@ def normalize3D(x):
    return x/np.kron(np.ones((1, 3)), utils.mkvc(length3D(x), 2))


-class CurvilinearMesh(BaseRectangularMesh, DiffOperators, InnerProducts, CurviView):
+class Array(properties.Array):


This is the implementation of an array that doesn't care about shape (I allow shape to be '*' which could be a 1D, 2D or 3D array). See discussion on seequent/properties#192

Union property could work well - see my comment on seequent/properties#192

lheagy · 2017-09-02T17:22:47Z

discretize/TreeMesh.py

+        min=0
+    )
+
+    cells = properties.Set(


I would prefer to keep this a private variable _cells. See discussion on seequent/properties#191

lheagy · 2017-09-02T17:23:31Z

discretize/TreeUtils.c

@@ -1,8 +1,8 @@
-/* Generated by Cython 0.24.1 */
+/* Generated by Cython 0.25.2 */


If you prefer, I can remove this. Updated cython cc @rowanc1

lheagy · 2017-09-02T17:24:33Z

tests/base/test_tensor.py

@@ -19,7 +19,6 @@ def setUp(self):
    def test_vectorN_2D(self):
        testNx = np.array([3, 4, 5, 6])
        testNy = np.array([5, 6, 8])
-


just whitespace in this file

rowanc1

Looking good. A few minor comments on the min/max side of things.

rowanc1 · 2017-09-02T20:38:28Z

discretize/BaseMesh.py

+    n = properties.Array(
+        "number of cells in each direction (dim, )",
+        dtype=int,
+        required=True,


Do we want min_length or max_length here?

http://propertiespy.readthedocs.io/en/latest/content/container.html#properties.List

Actually, that is only for lists. Perhaps we could change properties to have a set of tuples for defining the shape?

shape={(1,), (2,), (3,)}??

That would be handy

rowanc1 · 2017-09-02T20:39:17Z

discretize/BaseMesh.py

+    @properties.validator('n')
+    def check_n_shape(self, change):
+        change['value'] = np.array(change['value'], dtype=int).ravel()
+        if len(change['value']) > 3:


I think this can be solved by min/max length in the list property.

This isn't a list (it is an array). Which I don't think has a min/max length

rowanc1 · 2017-09-02T20:43:23Z

discretize/BaseMesh.py

    """

+    # Properties
+    n = properties.Array(


Can we change this later? Having it as a public property seems to suggest that we can change it....

the user shouldn't be changing it. It might, for example, get updated if you refine an octree mesh.
Properties doesn't currently allow private variables. see seequent/properties#191

agreed though - ideally, this should remain a private variable

rowanc1 · 2017-09-02T20:45:00Z

discretize/CurvilinearMesh.py

+        Array(
+            "node locations in a dimension",
+            shape='*'
+        ),


min=2 and max=3 length on the list?

This is an array, so we can't, but I will implement the Union Property as suggested by @fwkoch

fwkoch

Overall, I love the properties stuff - let me know if you have questions on any of my comments. I also had a few non-properties questions, mostly minor things. Also, I really like the whitespace cleanup everywhere!!

fwkoch · 2017-09-05T04:53:18Z

discretize/TreeMesh.py

+            "cell number",
+            min=0
+        ),
+        default=set()


I think you should use default=set - this dynamic default will get called every time, creating a new set for each TreeMesh instance. The way you have it now, this specific set() will be shared across all instances, I think.

fwkoch · 2017-09-05T05:05:10Z

discretize/BaseMesh.py

+            )
+
+    @properties.validator('x0')
+    def check_x0_vs_n(self, change):


A couple picky thoughts on this:

self.n could be None if someone deletes it or sets it to undefined. len(None) would give an ugly error. Possibly worth adding an extra sanity check for that?

Also, possibly worth running this check in the other direction (ie you validate x0 vs n, but not n vs x0) - may come up depending on how someone is using this.

fwkoch · 2017-09-05T05:06:43Z

discretize/BaseMesh.py

@@ -44,7 +60,7 @@ def dim(self):
        :rtype: int
        :return: dim
        """
-        return self._dim
+        return len(self.n)


For all of these, actually, possibly worth checking if things (self.n, self.dims, etc.) are None.

fwkoch · 2017-09-05T05:12:04Z

discretize/CurvilinearMesh.py

@@ -25,7 +27,62 @@ def normalize3D(x):
    return x/np.kron(np.ones((1, 3)), utils.mkvc(length3D(x), 2))


-class CurvilinearMesh(BaseRectangularMesh, DiffOperators, InnerProducts, CurviView):
+class Array(properties.Array):


Union property could work well - see my comment on seequent/properties#192

fwkoch · 2017-09-05T05:14:18Z

discretize/CurvilinearMesh.py

-            assert nodes_i.shape == nodes[0].shape, ("nodes[{0:d}] is not the "
-                                                     "same shape as nodes[0]"
-                                                     .format(i))
+    def __init__(self, nodes=nodes, **kwargs):


Looks like you are using nodes, the properties.List, as the default value...? Seems fishy... though I may be missing something.

thanks, this is a typo.

fwkoch · 2017-09-05T05:22:24Z

discretize/CylMesh.py

+
+    @properties.validator('cartesianOrigin')
+    def check_cartesian_origin_shape(self, change):
+        change['value'] = np.array(change['value'], dtype=float).ravel()


This is cool! I like seeing that you are using the validator to (possibly) change the value.

fwkoch · 2017-09-05T05:31:54Z

discretize/TensorMesh.py

+
+        else:
+            assert 'h' in kwargs.keys(), 'cell widths must be provided'
+            h = kwargs.pop('h')


This is a little funny - on first impulse, my question was "Why does h get special treatment as an arg but not as a kwarg?" Looking into it further, though, as long as h is provided as an arg or a kwarg, it will set h_in. So really, this line will never be reached; if the else clause is reached, the assert will fail (I think at least).

Silly example:

It is a bit odd, and maybe you have a suggestion for me on how this might be made more transparent?

The reason for this is to support two uses:

creating the mesh with h, x0 as args (or h_in). This is currently how most people work with the API

the reason for having h also allowed as a kwarg is so that I can create a re-create a mesh from a serialized mesh

where load_mesh deserializes the mesh (https://github.com/simpeg/discretize/blob/ref/properties/discretize/utils/meshutils.py#L17)

def load_mesh(filename): """ Open a json file and load the mesh into the target class As long as there are no namespace conflicts, the target __class__ will be stored on the properties.HasProperties registry and may be fetched from there. :param str filename: name of file to read in """ with open(filename, 'r') as outfile: jsondict = json.load(outfile) data = properties.HasProperties.deserialize(jsondict, trusted=True) return data

and for this I am under the impression that all properties in the serialized json needed to be able to be read in as kwargs.

I see the need for h as both arg and kwarg - I think that is still covered.

This exposes a bit of a subtlety with how Python treats args/kwargs. If a named arg variable shows up anywhere in the kwargs (and is not provided in the args) it is assigned to the associated arg (and that variable does not show up in kwargs internal to the function).

If you play around with this:

class Blah(object): def __init__(self, h=None, x0=None, **kwargs): print(h) print(x0) print(kwargs)

You will find for all of these, kwargs = {'something_else': 2} (and h/x0 are assigned separately):

Blah(0, 1, something_else=2) Blah(h=0, x0=1, something_else=2) Blah(something_else=2, x0=1, h=0)

This errors (h assigned twice):

Blah(0, x0=1, something_else=2, h=3)

So bottom line, even if it is coming in as a deserialized dict, the argument will still be moved to the right place. Does that make sense...?

fwkoch · 2017-09-05T05:38:35Z

discretize/TensorMesh.py


-        # Ensure h contains 1D vectors
-        self._h = [utils.mkvc(x.astype(float)) for x in h]
+        n = np.array([x.size for x in h])


Should this be else?

fwkoch · 2017-09-05T05:48:10Z

discretize/utils/meshutils.py

+    """
+    Open a json file and load the mesh into the target class
+
+    As long as there are no namespace conflicts, the target __class__


If you want, you can narrow down your namespace a little by overriding the _REGISTRY on your BaseMesh class (ie just add _REGISTRY = {} to the class). Then you could say data = BaseMesh.deserialize(...) and it will only look at classes that inherit from BaseMesh (and therefore are on the BaseMesh registry, rather than the HasProperties registry).

Ok, cool, thanks!

…the testing to make sure errors are raised if the user messes with n.

…rialize onto BaseMesh rather than properties.HasProperties

…sh. See updates in seequent/properties#195

lheagy · 2017-09-11T07:36:34Z

Properties can now be private (see seequent/properties#194, thanks @fwkoch!!), so...

on the BaseMesh n is a private property (_n)
on the TreeMesh, levels and cells are private (_levels and _cells)

and Arrays can now be given a set for the shape (see seequent/properties#195), so

on the CurviMesh, nodes is a list of arrays.

I am happy with the state of this pr at this point, @rowanc1, if you are as well, I will do a beta release and merge to dev.

rowanc1

Looks good to me! Thanks @lheagy this is awesome.

rowanc1 · 2017-09-18T19:12:16Z

discretize/BaseMesh.py

@@ -290,11 +351,33 @@ def projectEdgeVector(self, eV):
        ), 'eV must be an ndarray of shape (nE x dim)'
        return np.sum(eV*self.tangents, 1)

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


This might want to be on a base class for simpeg in future (or in the utils of discretize)? It looks like a pretty generic function.

Agreed. Once we have injected properties elsewhere in SimPEG we can abstract this up.

rowanc1 · 2017-09-18T19:13:01Z

discretize/BaseMesh.py

+            "You cannot change types when reshaping."
+        )
+        assert xType in outType, "You cannot change type of components."
+
        if type(x) == list:
            for i, xi in enumerate(x):


It looks like this never gets hit by tests. Did it before?

The only thing I changed above is whitespace. This particular line has not changed at all in this pr

rowanc1 · 2017-09-18T19:13:52Z

discretize/CurvilinearMesh.py

+        "List of arrays describing the node locations",
+        prop=properties.Array(
+            "node locations in an n-dimensional array",
+            shape={('*', '*'), ('*', '*', '*')}


lheagy added 10 commits July 6, 2017 08:52

Merge pull request #68 from simpeg/dev

fa11a0a

Dev

use properties for n, x0 for the base mesh

145bf00

use n instead of private variable

19882a2

whitespace cleanup in base mesh

af1035d

explicitly add x0 as an input to the basemesh

693c5d0

esure x0 being passed in init of BaseRectangularMesh

809a149

minimal injection of properties to base tensor mesh

44fea79

have tree mesh look at top level h rather than a hidden property

d688b85

add ability to save and load tensor meshes

209856a

add a copy method

85a4ba0

lheagy requested review from rowanc1 and fwkoch September 1, 2017 15:50

lheagy added 6 commits September 1, 2017 09:12

fix instantiation of CylMesh so that cartesianOrigin is now a propert…

5fc99d5

…y, add a check to ensure the cartesian origin is being set properly. Have x0 be passed as a kwarg in meshIO

pass x0 as a kwarg

d9d59cc

make levels a property in tree mesh. Pass x0 as a kwarg

2606404

pass x0 as a kwarg in view

24accb0

add properties docs to intersphinx, remove quantified code from readme

d347514

remove requirement that x0 be passed as a kwarg

7b03487

lheagy added 3 commits September 1, 2017 17:46

check serialization of cyl mesh, allow kwargs in init of tensor meshes

f8c9c62

make _cells a public variable so that it can be a property (see seequ…

393851f

…ent/properties#191, test serialization and deserialization of tree mesh

add tests to check edges, volumes and areas are still the same

e770ff9

lheagy mentioned this pull request Sep 2, 2017

default shape of properties.Array seequent/properties#192

Closed

add a subclass of the properties Array to overcome the size issue dis…

8b061fe

…cussed in seequent/properties#192

lheagy commented Sep 2, 2017

View reviewed changes

lheagy added 3 commits September 2, 2017 10:53

add docstring for Array. add nitpick ignore for docs

f471d17

set in the meshIO rather than for reading in UBC meshes

fd85204

add a few more property checks in the testing

abee619

rowanc1 reviewed Sep 2, 2017

View reviewed changes

fwkoch reviewed Sep 5, 2017

View reviewed changes

lheagy added 12 commits September 5, 2017 12:46

use the union property for an arbitrary array size

5507fc1

whitespace cleanup in TensorMesh

2930503

remove duplication in args / kwargs of h

d76cd60

more robust validation on n in the BaseMesh, check some end-cases in …

018d585

…the testing to make sure errors are raised if the user messes with n.

move to MeshIO. import load_mesh into base discretize namespace. dese…

5060219

…rialize onto BaseMesh rather than properties.HasProperties

update pypi credentials

5980f43

Bump version: 0.1.9 → 0.1.10b0

a72837e

make n a private property

23b5f45

cells is a private variable

76d78c5

update requirements to rely on properties 0.3.6b0

d15dda3

levels is a private property of the tree mesh

ad2d77a

use the array property with shapes as a set for nodes in the curvi me…

a75afa5

…sh. See updates in seequent/properties#195

rowanc1 approved these changes Sep 18, 2017

View reviewed changes

lheagy merged commit 267c9fd into dev Sep 19, 2017

lheagy deleted the ref/properties branch September 19, 2017 08:35

lheagy mentioned this pull request Sep 21, 2017

discretize 0.1.10 #73

Merged

3 tasks

		@@ -1,8 +1,8 @@
		/* Generated by Cython 0.24.1 */
		/* Generated by Cython 0.25.2 */

Ref/properties #70

Ref/properties #70

Conversation

lheagy commented Sep 1, 2017 • edited Loading

Implement properties in discretize

todos

Related issues

codecov bot commented Sep 1, 2017 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rowanc1 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lheagy Sep 2, 2017 • 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

fwkoch left a 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

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

fwkoch Sep 5, 2017 • 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

lheagy commented Sep 11, 2017

rowanc1 left a 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

Choose a reason for hiding this comment

lheagy commented Sep 1, 2017 •

edited

Loading

codecov bot commented Sep 1, 2017 •

edited

Loading

lheagy Sep 2, 2017 •

edited

Loading

fwkoch Sep 5, 2017 •

edited

Loading