Skip to content

Conversation

@mariusnevland
Copy link
Contributor

Proposed changes

This PR aims to improve the tutorials. This includes rewriting and reordering certain parts, as well as removing some parts altogether.

Types of changes

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

  • Minor change (e.g., dependency bumps, broken links).
  • Bugfix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).
  • Testing (contribution related to testing of existing or new functionality).
  • Documentation (contribution related to adding, improving, or fixing documentation).
  • Maintenance (e.g., improve logic and performance, remove obsolete code).
  • Other:

Checklist

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

  • The documentation is up-to-date.
  • Static typing is included in the update.
  • This PR does not duplicate existing functionality.
  • The update is covered by the test suite (including tests added in the PR).
  • If new skipped tests have been introduced in this PR, pytest was run with the --run-skipped flag.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

"cell_type": "markdown",
"metadata": {},
"source": [
"## Geometry\n",
Copy link
Contributor

@keileg keileg Dec 3, 2025

Choose a reason for hiding this comment

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

'across more than one dimension' - Rather it is a generalization that applies to any dimensions, also the ambient one.

The specific volume is not necessarily defined as a^N-d, but this is a common model to apply.

For fracture intersections, the specific volume can be a^2 or ^3.


Reply via ReviewNB

@@ -245,43 +245,7 @@
"source": [
Copy link
Contributor

@keileg keileg Dec 3, 2025

Choose a reason for hiding this comment

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

Is this really the first real tutorial the user should have a look at? If we think of the multiphysics models as the entry point, the user will not need to generate grids manually, and although it is useful to know how to get hold of geometric information, it is not on my top ten list of things everybody should know how to do. Plotting is of course a different matter, and it is also very important that the users are aware there are both simplex and Cartesian grids.

I think the question underlying this comment and my somehow related question to the introduction is, do we have a clear idea for which direction the user should be pushed? Is there more than one such natural way, and do we have the capacity to create and maintain such multiple ways if relevant?


Reply via ReviewNB

"Similarly, fracture intersections are represented by one-dimensional lines and intersections of fracture intersections by zero-dimensional points. \n",
"The same principle holds also for two-dimensional domains: the fractures are one-dimensional lines and fracture intersections are zero-dimensional points. \n",
"To account for the collapsed area and volume, we introduce a _specific volume_ [m$^{3-d}$], with $d$ denoting the dimension of the subdomain in question. \n",
"For more information regarding the specific volume we refer to the [conventions](https://github.com/pmgbergen/porepy/blob/develop/tutorials/conventions.ipynb) tutorial.\n",
Copy link
Contributor

@keileg keileg Dec 3, 2025

Choose a reason for hiding this comment

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

Except from the grid, most of the 'key components' are not something a new user should look into in the first go, although they will eventually need to learn about most if not all items on the list. The model classes, which ought to be the entry point for users, are much less prominent in this text. I am not sure how to do this, but I do think that a restructuring will be beneficial. Let's discuss this.

One observation, which may or may not be useful in this context, is that the list of components do give the link to the main components of a set of model equations: Geometry, grid, discretization, and dealing with the non-linear system.


Reply via ReviewNB

@@ -21,7 +21,7 @@
"For most simulation purposes, the final mixed-dimensional grid is all that is needed. Therefore, we start by showing a shortcut for obtaining a `MixedDimensionalGrid` given a set of fractures, a domain and mesh size parameters. All these will be described in more detail below.\n",
Copy link
Contributor

@keileg keileg Dec 3, 2025

Choose a reason for hiding this comment

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

First, this introductory paragraph I liked a lot, it is easy to see how that relates to the roadmap that was partially indicated in the first tutorial.

Regarding the edit, I am not sure we do anyone a favor by sending them into that particular folder.


Reply via ReviewNB

@@ -21,7 +21,7 @@
"For most simulation purposes, the final mixed-dimensional grid is all that is needed. Therefore, we start by showing a shortcut for obtaining a `MixedDimensionalGrid` given a set of fractures, a domain and mesh size parameters. All these will be described in more detail below.\n",
Copy link
Contributor

@keileg keileg Dec 3, 2025

Choose a reason for hiding this comment

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

General comment: Please avoid committing changes to notebooks when there is no actual change to the output.


Reply via ReviewNB

@@ -21,7 +21,7 @@
"For most simulation purposes, the final mixed-dimensional grid is all that is needed. Therefore, we start by showing a shortcut for obtaining a `MixedDimensionalGrid` given a set of fractures, a domain and mesh size parameters. All these will be described in more detail below.\n",
Copy link
Contributor

@keileg keileg Dec 3, 2025

Choose a reason for hiding this comment

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

I think this is fair, we will need to revise this part anyhow in connection with issue 1518.


Reply via ReviewNB

@keileg
Copy link
Contributor

keileg commented Dec 3, 2025

Partial review. I suggest we have a chat on how to approach this, but I will need some more time for thinking before that.

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.

4 participants