Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

YAML specification of diffusion-reaction models #267

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

LawrenceDior
Copy link

read_tracer_from_yml reads tracer diffusivities and reaction terms from a .yml file and returns an OrderedDict of Firedrake and UFL objects representing this information.

The ADR_Model class is responsible for model validation and the parsing of reaction terms.

The load_tracers_2d method of the FlowSolver2d class calls read_tracer_from_yml and adds the corresponding tracers to the FlowSolver2d object.

examples/reaction/gray_scott_io.py contains a working example of a diffusion-reaction model loaded from disk.

See reaction_models/gray-scott.yml for an example of a .yml file in the expected format.

thetis/adr_model.py Outdated Show resolved Hide resolved
Comment on lines 35 to 38
reserved_symbols = {
"pi", "e",
"x", "y", "z", "t",
"u", "u_x", "u_y", "u_z"}
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess a discussion point is whether we want all of these (and any more) as reserved symbols. I can imagine pi, e, x, y, t being useful for if you have a source term that depends on them. Probably not z in a 2D model.

Copy link
Author

Choose a reason for hiding this comment

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

Agreed -- see separate comment. For z, I suppose the question is whether or not this class might also be used in tandem with 3D solvers.

@@ -3,3 +3,6 @@ netCDF4
pyproj
uptide
pytz
networkx
pyyaml
sympy
Copy link
Contributor

Choose a reason for hiding this comment

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

At the minute the implementation converts yml->dict->sympy->UFL. Maybe it is possible to do yml->dict->UFL? If so, it's probably best to leave for future work.

Comment on lines 35 to 38
reserved_symbols = {
"pi", "e",
"x", "y", "z", "t",
"u", "u_x", "u_y", "u_z"}
Copy link
Author

@LawrenceDior LawrenceDior Sep 10, 2021

Choose a reason for hiding this comment

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

These symbols are reserved to allow reaction terms to contain pi, e, spatial coordinates, time, the flow field, and individual components of the flow field.

I'm not aware of any issues with pi or e, because these are just constants. However, at present, parse_tracers_from_dict will not work properly if it encounters a reaction term containing any of these other symbols. It expects all symbols to correspond to either constants or species.

The simplest way to solve this problem would be to remove everything except pi and e from reserved_symbols, but it might be worth considering alternatives if people would like to be able to include spatial coordinates, etc. in reaction terms.

thetis/adr_model.py Outdated Show resolved Hide resolved
thetis/adr_model.py Outdated Show resolved Hide resolved
thetis/utility.py Outdated Show resolved Hide resolved
thetis/utility.py Outdated Show resolved Hide resolved
@jwallwork23
Copy link
Contributor

Note: this could also be used for Thetis' sediment model.

Copy link
Contributor

@jwallwork23 jwallwork23 left a comment

Choose a reason for hiding this comment

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

Thanks, Lawrence! This is great. A few comments and requests. e.g. it'd be nice if you could demonstrate the networkx usage in the Gray-Scott example.

Sorry it took a while to get round to reviewing.

thetis/adr_model.py Outdated Show resolved Hide resolved
thetis/adr_model.py Outdated Show resolved Hide resolved
thetis/adr_model.py Outdated Show resolved Hide resolved
thetis/adr_model.py Outdated Show resolved Hide resolved
self.species[k] = {
"name": v["name"],
"index": i,
"diffusion": float(diffusion_term),
Copy link
Contributor

Choose a reason for hiding this comment

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

So at the minute we are only supporting constant diffusion terms, which is fine. To support spatially varying diffusivity and coefficients, I guess we would need the functionality to load from file or something. Probably best to just leave for now until someone needs to be able to do it.

thetis/adr_model.py Outdated Show resolved Hide resolved
thetis/utility.py Outdated Show resolved Hide resolved
thetis/utility.py Outdated Show resolved Hide resolved
thetis/utility.py Outdated Show resolved Hide resolved
jwallwork23 and others added 9 commits February 16, 2022 14:47
- Added networkx to requirements.txt (required for ADR_Model.dependency_graph)
- Adjusted the way load_tracers_2d sets the name and fname parameters of add_tracer_2d
- The preserve_order argument of read_tracer_from_yml is now True by default
- Corrected read_tracer_from_yml docstring
@jwallwork23
Copy link
Contributor

With @LawrenceDior's permission, I made the requested changes.

jwallwork23 added a commit that referenced this pull request Apr 8, 2022
jwallwork23 added a commit that referenced this pull request Apr 21, 2022
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.

2 participants