pmgbergen / porepy

Python Simulation Tool for Fractured and Deformable Porous Media
GNU General Public License v3.0
251 stars 88 forks source link

Reimplemented method cell_face_as_dense in the grid class #1268

Closed keileg closed 3 days ago

keileg commented 4 days ago

Proposed changes

Changes:

  1. Replaced the (8 year old!) implementation of cell_face_as_dense in the Grid class. The new implementation is much more transparent, and documented.
  2. Renamed the method cell_face_as_dense -> cell_faces_as_dense.
  3. Improved the test for this method.

@IvarStefansson @IngridKJ: Although the test suite is much improved, the old implementation did not fail on this expanded set. I am not sure which grid you used to make the method fail today, but could you test the new version and see if it works? If so, I am not sure if we should expand the test suite with the previously failing grid, so let me know what you think. If it still fails, we will definitely add it.

Types of changes

What types of changes does this PR introduce to PorePy? Put an x in the boxes that apply.

Checklist

Put an x in the boxes that apply or explain briefly why the box is not relevant.

review-notebook-app[bot] commented 4 days ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

IngridKJ commented 3 days ago

I performed a quick check with my previously failing grid, and things seem to be working fine now. What I did was simply running my script in the new branch, and I do not get the dimension mismatch I got previously. So, the grid seems to be working fine now. Are you interested in the details or code for the grid which failed, @keileg?

keileg commented 3 days ago

Are you interested in the details or code for the grid which failed, @keileg?

No, I think we are good. Do you agree, @IvarStefansson?

IvarStefansson commented 3 days ago

Are you interested in the details or code for the grid which failed, @keileg?

No, I think we are good. Do you agree, @IvarStefansson?

Yes. Merging. Thanks for fixing, @keileg!