Add fenicsx and simplified Landlab solvers - #889
Conversation
|
Thank you for the updates, @joargu! Since I see that the PR description is missing and the CI is failing, I marked this as draft. Let me know once you need a review, or if you need any help. |
uekerman
left a comment
uekerman
left a comment
There was a problem hiding this comment.
Thanks for the update!
Structure looks good, a few minor comments below. I have not yet run things, but I want to do so once the README is updated.
| self.status[1:-1, 1:-1] = self.CORE | ||
|
|
||
| # Have one open boundary on the south side | ||
| # rmg.set_closed_boundaries_at_grid_edges(True, True, True, False) |
There was a problem hiding this comment.
It's often better not to add commented-out code. This will soon be hard to understand.
| # order: right, top, left, bottom | ||
| self.status[:, -1] = self.CLOSED | ||
| self.status[-1, :] = self.CLOSED | ||
| self.status[:, 0] = self.CLOSED |
There was a problem hiding this comment.
seems like one of the four is missing
| for n1, n2 in self.links | ||
| ]) | ||
|
|
||
| internal_dt = 0.15 * self.dx * self.dx / np.nanmax(active_D) |
There was a problem hiding this comment.
What is the 0.15 here? Where does the value come from?
| repeats = int(dt // internal_dt) | ||
| remainder = dt - repeats * internal_dt |
There was a problem hiding this comment.
What are these lines doing? Please add comments.
MakisH
left a comment
MakisH
left a comment
There was a problem hiding this comment.
I know this is still a draft, but I did a first review pass as well. Happy to look over it again later.
There was a problem hiding this comment.
The clean.sh and run.sh scripts are not marked as executable. Mark them with:
chmod +x clean.sh run.sh
(for both case directories)
|
|
||
| # Divide the timestep dt passed to the diffuser into internal stability-limited substeps. | ||
| # Perform as many full substeps as possible (repeats), followed by one smaller substep for the remainder. | ||
| internal_dt = ALPHA * self.dx * self.dx / np.nanmax(active_D) |
There was a problem hiding this comment.
I got an error here:
---[precice] time-window 1, t 0 (max: 100), Dt 2, max-dt 2
Traceback (most recent call last):
File "/home/gc/repos/precice/tutorials/wolf-sheep-soil-creep/soil-creep-simplified-landlab/soil_creep.py", line 182, in <module>
diffuser.step(dt)
~~~~~~~~~~~~~^^^^
File "/home/gc/repos/precice/tutorials/wolf-sheep-soil-creep/soil-creep-simplified-landlab/soil_creep.py", line 72, in step
internal_dt = ALPHA * self.dx * self.dx / np.nanmax(active_D)
^^^^^
NameError: name 'ALPHA' is not defined. Did you mean: 'self.ALPHA'?
There was a problem hiding this comment.
Add also (in both requirements.txt) the Python bindings explicitly.
Necessary for the system tests to be able to adjust the pyprecice version.
pyprecice~=3.0
There was a problem hiding this comment.
self-note: I could not install FEniCSx on my system at the moment. In any case, we should test it on the system tests once the respective entries are there (see the checklist in the PR description).
3 participants
TODO: Summarize and motivate the changes, link to issues, remove the checklist entries that are not relevant.
Checklist
changelog-entries/<PRnumber>.md.requirements.txtfiles, I regenerated siblingrequirements-reference.txtfiles withpython3 tools/releasing/update-requirements-reference.py(pass a path to update only that directory, or--allto refresh everything).For new tutorials or tutorial cases:
README.mdmetadata.yamltools/tests/tests.yaml_config.yaml