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

Entry barrier for setting up simple models like heat equation is too high #1174

Closed
vlipovac opened this issue May 31, 2024 · 4 comments
Closed

Comments

@vlipovac
Copy link
Contributor

This issue came up during an advisory meeting with a trainee at the CSD, and consists of two parts.

  1. The module porepy.models.energy_balance.py is designed to be an addition to flow and transport models, not to be a stand-alone module for solving the heat equation. Some mixins and inheritance relations are missing (f.e. VariablesEnergyBalance does not inherit from VariablesMixin, while VariablesSinglePhaseFlow does)

  2. The model set-up requires too many unnecessary mixins, especially for the fixed-dimensional case. In the case of the heat equation, I identified two sections in the code causing this:
    a. The base models always call the methods assembling interface and well equations with an empty sequence of lower-dimensional grids. I observed this in all base models. This might also lead to confusion when inspecting the equation system, where empty equations are added.
    b. The solution strategy of the energy balance does not adapt to the case when there is no advective flux, calling methods like darcy_flux which need to be provided returning e.g. a wrapped zero array.

I am not sure though, if point 2.a is something which should be fixed, or if this is something the user has to keep in mind when setting up models.

@IvarStefansson
Copy link
Contributor

For now, like you say, the energy balance model is simply not designed for standalone use. I don't think there are imminent plans to redesign it. Do you think the documentation needs clarification on this point?

@vlipovac
Copy link
Contributor Author

vlipovac commented Jun 6, 2024

For now, like you say, the energy balance model is simply not designed for standalone use. I don't think there are imminent plans to redesign it. Do you think the documentation needs clarification on this point?

The documentation in the module docstring points that out, and I think it is fine.

What are your thoughts on the second point. that the models always provide all equations, even in the fixed-dimensional case?
Of course the total flops are not increased severely, since the arrays are empty, but they appear in the Operator tree (recursion during _evaluate) and in the equation system, despite being empty.

@keileg
Copy link
Contributor

keileg commented Jun 6, 2024

This is a fair question, which should be addressed as part of a larger maintenance of the equation and operator framework.

@keileg
Copy link
Contributor

keileg commented Jun 7, 2024

I wrote down some thoughts in this direction in #1184. @vlipovac is it okay with you if we close this issue and continue the discussion there?

@vlipovac vlipovac closed this as not planned Won't fix, can't repro, duplicate, stale Jun 7, 2024
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

No branches or pull requests

3 participants