-
Notifications
You must be signed in to change notification settings - Fork 28
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
base: master
Are you sure you want to change the base?
Conversation
thetis/adr_model.py
Outdated
reserved_symbols = { | ||
"pi", "e", | ||
"x", "y", "z", "t", | ||
"u", "u_x", "u_y", "u_z"} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
thetis/adr_model.py
Outdated
reserved_symbols = { | ||
"pi", "e", | ||
"x", "y", "z", "t", | ||
"u", "u_x", "u_y", "u_z"} |
There was a problem hiding this comment.
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.
Note: this could also be used for Thetis' sediment model. |
There was a problem hiding this 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
self.species[k] = { | ||
"name": v["name"], | ||
"index": i, | ||
"diffusion": float(diffusion_term), |
There was a problem hiding this comment.
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.
- 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
454d657
to
b25919d
Compare
With @LawrenceDior's permission, I made the requested changes. |
read_tracer_from_yml
reads tracer diffusivities and reaction terms from a .yml file and returns anOrderedDict
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 theFlowSolver2d
class callsread_tracer_from_yml
and adds the corresponding tracers to theFlowSolver2d
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.