Conversation
rtimms
left a comment
rtimms
left a comment
There was a problem hiding this comment.
Looking good so far. Next steps are:
- Add an example of solving the 3D heat eqn in a jelly and prismatic domain, with different boundary conditions and a prescribed heat souce
- Add tests for the 3d finite volume methods (make sure things are discredited as expected, check simple problems are solved correctly). To help with this you can use the "method of manufactured solutions".
Once the mesh and method are tested for simple general equations we can start on the battery-specific parts of the project in a separate PR
Rishab87
changed the title
|
I am facing few issue with BCs, few tests are failing mostly due to shape mismatches, I saw @aabills was implementing 2D FVM, I was following his pattern. So it would be great if Alec can have a look at this PR specifically around ghost nodes and neumann values and shift. And overall too if you can review the PR to see if there is any potential issues present due to which tests are failing. |
|
Hey Rishab, yeah Ghost nodes are a real pain. Let me wrap mine up this week and I'll get back to you ASAP |
|
thanks @aabills, in meantime I'll try to fix these errors myself. |
| z: {"min": pybamm.Scalar(0), "max": pybamm.Scalar(1)}, | ||
| } | ||
| } | ||
| var_pts = {x: 5, y: 5, z: 5} |
There was a problem hiding this comment.
do we need var_pts here since the mesh generator decides on the number of nodes based on h?
it would be nice if you could grab the var_pts after the mesh is created so you know how many nodes you have in x/y/z
There was a problem hiding this comment.
Yeah we can grab that with .npts, we cant remove var_pts here since it is a compulsory argument in pybamm.Mesh
There was a problem hiding this comment.
OK, I see. We may want to pass var_pts anyway since other domains may not be in 3D. Can you add a warning if the submesh type is 3D FEM saying that var_pts will be ignored and the mesh will be created to meet the h tolerance. And maybe recommend passing None for spatial variables on a 3D domain
tests/shared.py
Outdated
| z = pybamm.SpatialVariable("z", ["current collector"]) | ||
| geometry = { | ||
| "current collector": { | ||
| x: {"min": pybamm.Scalar(0), "max": pybamm.Scalar(1)}, |
There was a problem hiding this comment.
for testing I would make these have different lengths to avoid accidentally covering up errors where an axis gets swapped
tests/unit/test_spatial_methods/test_scikit_finite_element_3d.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Robert Timms <43040151+rtimms@users.noreply.github.com>
|
@rtimms I've addressed all the reviews can you re-review it again? |
| z: {"min": pybamm.Scalar(0), "max": pybamm.Scalar(1)}, | ||
| } | ||
| } | ||
| var_pts = {x: 5, y: 5, z: 5} |
There was a problem hiding this comment.
OK, I see. We may want to pass var_pts anyway since other domains may not be in 3D. Can you add a warning if the submesh type is 3D FEM saying that var_pts will be ignored and the mesh will be created to meet the h tolerance. And maybe recommend passing None for spatial variables on a 3D domain
|
@rtimms I've addressed all reviews, can you please re-review it again? |
|
@aabills any updates on this? |
5 participants
Description
This PR will add a support for 3D FEM, 3D meshing along with a mesh generator with support for prismatic and cylindrical geometry
Fixes # (issue)
Type of change
Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #)
Important checks:
Please confirm the following before marking the PR as ready for review:
nox -s pre-commitnox -s testsnox -s doctests