simpeg / discretize

Discretization tools for finite volume and inverse problems.
http://discretize.simpeg.xyz/
MIT License
165 stars 34 forks source link

Add new TensorCell class #325

Closed santisoler closed 1 year ago

santisoler commented 1 year ago

The new TensorCell class represents a single cell in a TensorMesh, with center, bounds, neighbors, nodes, edges and faces properties. Add __iter__ and __getitem__ methods to TensorMesh. The latter allows to obtain a single cell in the mesh based on their indices. The former allows to iterate over every cell in the mesh. Add tests for the new class and methods.

jcapriot commented 1 year ago

I'm trying to think if it would be good to just not allow a User to directly instantiate a TensorCell , We could do this by defining a __new__ method and interacting with that internally, then throwing an exception in the class's __init__.

santisoler commented 1 year ago

It sounds a little too hacky... I doubt that if we don't provide examples that manually instantiate a TensorCell, users won't do it. And even if they do it, what would be the purpose? What could go potentially wrong with that?

jcapriot commented 1 year ago

Essentially it means you don't explicitly need to do all of the error checking for proper input. There's nothing wrong about creating the object on it's own, it's just that it does really have any tangible meaning without the Mesh class that it came from.

santisoler commented 1 year ago

I agree that this new class doesn't introduce anything new if it's isolated from the mesh. That's why I think users won't be manually creating TensorCells.

If I understand you correctly, you think it doesn't worth adding all the error raising on invalid inputs because users won't be creating this objects manually, so their proper creation should be covered by tests for the TensorMesh.__getitem__() method, right?

jcapriot commented 1 year ago

If I understand you correctly, you think it doesn't worth adding all the error raising on invalid inputs because users won't be creating this objects manually, so their proper creation should be covered by tests for the TensorMesh.__getitem__() method, right?

Yes, exactly. Unless we are concerned about ourselves programming this incorrectly to start with I don't think we necessarily need all the checks.

jcapriot commented 1 year ago

Also, if the cell knows the shape of the mesh it came from, it can determine for itself the neighbor, node, face, etc. indices without passing it the mesh.

santisoler commented 1 year ago

Also, if the cell knows the shape of the mesh it came from, it can determine for itself the neighbor, node, face, etc. indices without passing it the mesh.

Sounds good, that's something I can add.

codecov[bot] commented 1 year ago

Codecov Report

Merging #325 (58b8e55) into main (2480627) will increase coverage by 0.18%. The diff coverage is 96.21%.

@@            Coverage Diff             @@
##             main     #325      +/-   ##
==========================================
+ Coverage   85.21%   85.39%   +0.18%     
==========================================
  Files          38       39       +1     
  Lines       11578    11763     +185     
==========================================
+ Hits         9866    10045     +179     
- Misses       1712     1718       +6     
Impacted Files Coverage Δ
discretize/tensor_mesh.py 84.71% <94.20%> (+2.82%) :arrow_up:
discretize/tensor_cell.py 97.39% <97.39%> (ø)
discretize/__init__.py 60.86% <100.00%> (+1.77%) :arrow_up:

... and 1 file with indirect coverage changes

santisoler commented 1 year ago

@jcapriot I think this is ready to be reviewed. Is there anything else we would like to add to the TensorCell class?

santisoler commented 1 year ago

For some reason, Windows machines didn't like to check if a variable is an integer with

np.issubdtype(type(variable), int)

But using np.integer instead solved the issue:

np.issubdtype(type(variable), np.integer)
jcapriot commented 1 year ago

That makes sense, int is not technically a numpy dtype