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

Add new TensorCell class #325

Merged
merged 41 commits into from
Jun 27, 2023
Merged

Add new TensorCell class #325

merged 41 commits into from
Jun 27, 2023

Conversation

santisoler
Copy link
Member

@santisoler santisoler commented Jun 2, 2023

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.

The new TensorCell class represents a single cell in a TensorMesh, with
`center` and `bounds` 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.
santisoler added 10 commits June 5, 2023 10:52
Use `numpy.unravel_index` to convert integer indices into tuples,
following the right FORTRAN ordering. Simplify `__iter__` by building
the iterator with a simple for loop over the whole range of cells, and
reply on `__getitem__` for the unravelling of the indices. Add a new
test that checks if the int indices return the expected cells.
Add a method to compare two TensorCell objects by their `h` and `origin`
attributes.
Add support for slices while indexing cells in a TensorMesh. Add tests
for the new feature.
Store the index of the cell in its parent TensorMesh.
This new method return a list of the neighboring cells in the mesh
for the current cell. Add tests for the new method.
Sanitize inputs of TensorClass checking that elements of h and origin
are numerical values, and elements of index are integers.

TODO: test it
@jcapriot
Copy link
Member

jcapriot commented Jun 6, 2023

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
Copy link
Member Author

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?

Also, raise errors if any value of h, origin or index are bools (bools
are considered to be Number and Integral).
@jcapriot
Copy link
Member

jcapriot commented Jun 6, 2023

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.

Allow to get cells through negative single index (ravelled index),
through negative indices per dimension and using negative indices in
slices.
@santisoler
Copy link
Member Author

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
Copy link
Member

jcapriot commented Jun 6, 2023

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
Copy link
Member

jcapriot commented Jun 6, 2023

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
Copy link
Member Author

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.

The `index` attribute now returns the raveled index. Add a new
`index_unraveled` attribute that returns the unraveled indices.
The `neighbors` now is a list of raveled indices of neighbouring cells.
Now the `bounds` and `center` properties return Numpy arrays, mimicking
the behaviour of `TreeCell.center`.
@codecov
Copy link

codecov bot commented Jun 16, 2023

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%) ⬆️
discretize/tensor_cell.py 97.39% <97.39%> (ø)
discretize/__init__.py 60.86% <100.00%> (+1.77%) ⬆️

... and 1 file with indirect coverage changes

@santisoler
Copy link
Member Author

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

@santisoler santisoler marked this pull request as ready for review June 19, 2023 23:06
@santisoler
Copy link
Member Author

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
Copy link
Member

That makes sense, int is not technically a numpy dtype

Copy link
Member

@jcapriot jcapriot left a comment

Choose a reason for hiding this comment

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

Looks good, only thing left to do is to actually add it to the documentation pages.

We will likely need to re-name the Tree Mesh Cells section in __init__.py to just be Mesh Cells and update the text of that section accordingly (and add the TensorCell) to that list.

Also can you add a simple example to the docs of the TensorCell ?

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.

2 participants