-
Notifications
You must be signed in to change notification settings - Fork 13
New model #702
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
base: main
Are you sure you want to change the base?
Conversation
…e of the settings) and fix lint
…have different interfaces; also use to enable passing different physics options to SIQN timestepper. Do not expect Tom to like...
| no_normal_flow_bc_ids (list, optional): a list of IDs of domain | ||
| boundaries at which no normal flow will be enforced. Defaults to | ||
| None. | ||
| """ |
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 think the kwargs are a good solution. We should probably also add something to docstring here about what happens to the kwargs and give some examples of the kwargs that can be passed
| if family is None: | ||
| extruded_mesh = hasattr(mesh, "_base_mesh") | ||
| if extruded_mesh: | ||
| cell = mesh._base_mesh.ufl_cell().cellname() |
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.
Note that this will be cellname for future
| elif cell == "triangle": | ||
| family = "BDM" | ||
| else: | ||
| raise ValueError(f"The mesh provided (or its base mesh if 3D) must have cells of type interval, quadrilateral or triangle, not {cell}.") |
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.
Petty point but could this be over 2 lines?
| ) | ||
|
|
||
|
|
||
| class Model(SIQNModel): |
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 had wondered if this should be DefaultModel? Maybe that's not the best name, but Model feels more like the name of a base class? (I was actually a bit confused by the role this played but you haven't had chance to do the docstrings yet!)
| _diffusion_schemes = [] | ||
| for field, _ in self.diffusion_options: | ||
| _diffusion_schemes.append( | ||
| BackwardEuler(self.domain, field) |
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 think this will also need the recovery options specifying
No description provided.