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

Outdated documentation in the Upwind class #1237

Open
pschultzendorff opened this issue Oct 15, 2024 · 3 comments
Open

Outdated documentation in the Upwind class #1237

pschultzendorff opened this issue Oct 15, 2024 · 3 comments
Assignees

Comments

@pschultzendorff
Copy link
Contributor

The method pp.numerics.fv.upwind.Upwind.discretize has quite outdated documentation. For example, the required keys in the parameter dictionary as well as the return values of the method are wrong. In addition, #1192 changed the signs of the boundary matrices, but the code comments reflect the old convention. The other methods seem better, but, e.g., Upwind.cfl (not sure if this is even actively used) indicates also wrong dictionary keys.

@pschultzendorff pschultzendorff self-assigned this Oct 15, 2024
@keileg
Copy link
Contributor

keileg commented Oct 15, 2024

Thanks for volunteering for this, @pschultzendorff. Short comments:

  • I don't think the method cfl has been used for years, so if you decide it is easier to delete it than to fix it, I for one will not object.
  • I am not sure if the upwind class will stay relevant when the compositional flow branch is introduced. Could you comment briefly, @vlipovac?

@vlipovac
Copy link
Contributor

  • I am not sure if the upwind class will stay relevant when the compositional flow branch is introduced. Could you comment briefly, @vlipovac?

It is used in the advective part of energy and transport equations. Considering also our recent discussion on having the possibility to run a simulation without re-discretizing MPFA, it will remain relevant.

On a sidenote, there are open inconsistencies or unclarities in that class regarding BC (it needs an own instance of BoundaryCondition and , to my understanding, that instance needs all boundary faces to be flagged as 'dir')

@pschultzendorff
Copy link
Contributor Author

Good to hear that it will stay relevant, as I definitely need it for my two-phase flow code :D

I assume you are referring to #760 regarding the inconsistencies @vlipovac? For my part, I also ran into some inconsistencies/unwanted behavior of the BC when using it for modeling flow and transport in the fractional flow formulation. However, there are some more or less hacky ways to get it to work the way I need, so this should not be part of the issue.

I will fix the documentation as best as possible.

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