Custom fenics mesh#677
Conversation
jhdark
added
enhancement
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## fenicsx #677 +/- ##
===========================================
+ Coverage 99.07% 99.09% +0.02%
===========================================
Files 24 24
Lines 1076 1107 +31
===========================================
+ Hits 1066 1097 +31
Misses 10 10 ☔ View full report in Codecov by Sentry. |
RemDelaporteMathurin
left a comment
RemDelaporteMathurin
left a comment
There was a problem hiding this comment.
I don't understand the need for a new class here?
The mesh setter/getter could go in F.Mesh
The only thing that this class does is temporarily store the meshtags that are then passed to HTransportProblem.
We could save us the trouble of maintaining/documenting another class (although small) and instead allow users to simply set the mesh, facet_meshtags and volume_meshtags attributes of HTransportProblem
If users want to provide their own mesh and mesh_tags, could they not simply do the following :
import festim as F
# make a fenics mesh and meshtags
# ...
my_problem = F.HTransportProblem()
my_problem.mesh = F.Mesh(custom_mesh)
my_problem.facet_meshtags = custom_facet_meshtags
my_problem.volume_meshtags = cutsom_volume_meshtags
....| self.surface_meshtags = surface_meshtags | ||
| self.volume_meshtags = volume_meshtags |
There was a problem hiding this comment.
In HTransportProblem the meshtags are facet_meshtags and volume_meshtags, I would keep the same naming
| my_mesh = F.CustomFenicsMesh( | ||
| mesh=mesh_1D, | ||
| surface_meshtags=my_surface_meshtags, | ||
| volume_meshtags=my_volume_meshtags, | ||
| ) | ||
|
|
||
| my_model = F.HydrogenTransportProblem(mesh=my_mesh) |
There was a problem hiding this comment.
The usage could simply be:
| my_mesh = F.CustomFenicsMesh( | |
| mesh=mesh_1D, | |
| surface_meshtags=my_surface_meshtags, | |
| volume_meshtags=my_volume_meshtags, | |
| ) | |
| my_model = F.HydrogenTransportProblem(mesh=my_mesh) | |
| my_mesh = F.Mesh(mesh_1D) | |
| my_model = F.HydrogenTransportProblem(mesh=my_mesh) | |
| my_model.facet_meshtags = my_surface_meshtags | |
| my_model.volume_meshtags = my_volume_meshtags |
There was a problem hiding this comment.
We could even simplify things further by doing:
my_mesh = mesh_1D
my_model = F.HydrogenTransportProblem(mesh=my_mesh)
my_model.facet_meshtags = my_surface_meshtags
my_model.volume_meshtags = my_volume_meshtagsThen, in HTransportProblem make the setter of the mesh attribute:
def mesh(self, value):
if isinstance(value, dolfinx.mesh.Mesh):
self._mesh = F.Mesh(value)
else:
....
RemDelaporteMathurin
left a comment
RemDelaporteMathurin
left a comment
There was a problem hiding this comment.
Just a couple of final small comments and we're good to go!
Co-authored-by: Rémi Delaporte-Mathurin <40028739+RemDelaporteMathurin@users.noreply.github.com>
|
Is this PR linked to #647? |
Yes! |
3 participants
Proposed changes
With this users will be able to give their own fenics mesh with custom mesh tags.
Types of changes
What types of changes does your code introduce to FESTIM?
Checklist
Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...