-
Notifications
You must be signed in to change notification settings - Fork 93
Tutorial fluid modelling #1385
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: develop
Are you sure you want to change the base?
Tutorial fluid modelling #1385
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Some high-level comments after the first readthrough:
|
Thanks for the feedback @mariusnevland. I'll have a look and keep your comments in mind. |
@@ -0,0 +1,935 @@ | |||
{ |
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 a bit more context will be useful here. Rough ideas (not complete):
- What do you mean by a component?
- Why do we need to care about the fluid properties (they are part of the transport equations etc.)
This should not take much space, and giving references to more information is certainly fair, but give a bit more context could be very useful for readers.
Would it be an idea to introduce one physical system, say, two-phase two-component water-CO2 that can be followed throughout the tutorial? Sorry if the answer is obviously not.
Question: What is meant by 'respective functionalities'?
Reply via ReviewNB
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 have amended this section keeping with regards to your comments.
About the accompanying model, that would require to introduce quite a bit which is not yet part of the core code, essentially I would need a thermodynamic model (either an EoS, or couple of new mixins as some heuristic, simple fluid properties). Then I need to know whether you want something like that being part of a tutorial.
@@ -0,0 +1,935 @@ | |||
{ |
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.
If it is important that the user understands the notion of reference components/phases, more information is needed. If not, can it go?
Reply via ReviewNB
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 tried to explain that in the following sentences.
@@ -0,0 +1,935 @@ | |||
{ |
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.
Perhaps move some of this towards the top, to gather such information when natural. Also 'overall fraction' must be defined somewhere.
Reply via ReviewNB
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 have introduced links to the respective quantities implemented in the code. The docstring contain sufficient information to make it clear what it is.
Is that fine?
Since this tutorial is a bit extensive, I am trying to not burden it further with formulas and definitions, but focus on the application of the code and link to the respective parts. The documentation of the compositional framework is very extensive, I want to make use of it.
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 agree with the overall idea, we are not Wikipedia. Since the links just bring me to the top of the file, perhaps it is better to give this as an instruction, such as 'an explanation can be found in the documentation, see here'.
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.
The link should lead directly to the respective line. Have you tried that in vs code or online?
On my machine, locally in VS code, it leads to the correct line.
@@ -0,0 +1,935 @@ | |||
{ |
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.
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.
As a pure mixin. Added some extra explanation, here and for MyFluid
.
@@ -0,0 +1,935 @@ | |||
{ |
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.
The first paragraph here is a partial answer to the question 'how can we simulate such systems', or 'what happens under the hood in such simulations' - but not necessary to answer how a non-trivial fluid can be modeled in PorePy. Both types of information is relevant, and both can be fit here, but the plan for when to communicate which type is not clear to me.
Reply via ReviewNB
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.
Let's discuss in person.
Most important: This will become extremely useful for everyone as we introduce more advanced fluid modeling. I only managed to read half the tutorial before I had to run, will try to cover the second half no later than Monday. I tried to keep the comments on a high level, it seems to me we should focus on getting the context and general disposition of the tutorial right before getting into details. |
@@ -0,0 +1,935 @@ | |||
{ |
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.
Here it should be clearer that the 'EoS approach' refers to the particular implementation in PorePy that wraps the cubic EoS and similar, and not the modeling choice of using an EoS.
Reply via ReviewNB
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 am not sure what you mean, because it is made for the choice of using an EoS.
Should I emphasize that in theory it can be used for any computations?
@@ -0,0 +1,935 @@ | |||
{ |
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.
This cell gives a broad answer to the question 'how can I can invoke the EoS framework'? It does not so much answer the related question 'what do I need to do to invoke the EoS framework?' The difference to me is that there are a few contracts I have to fulfill and concepts I have to deal with (some keywords are 'dependencies_of_phase_properties', 'PhysicalState', 'PhaseProperties', and a few others), that are not really introduced, let alone explained. I am not asking for an explanation of the full architecture of the representation of fluids, even if that turns out to be needed it should not be done her. On the other hand, is there a way to explain on a very high lever what are the main components involved in the above code, and how do they interact?
This is also a place where a concrete example would be good to explain how to go from a bunch of zero arrays (cf. the last return statement) to something representing a fluid. However, I realize now that such an example risks being contrived - let's discuss.
Reply via ReviewNB
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 have added comments in various code blocks introducing the dependencies, the physical state and the phase properties, with focus on explaining what needs to be done.
I left a few more comments. @vlipovac I suggest we discuss how to approach this PR at some point. |
|
Closes #1254
Proposed changes
Adding a tutorial for modeling of multiphase and multicomponent fluids.
Changing the configuration of phases to omit the default
EquationOfState
and hence simplifying the setup for models not relying on EoS computations.Types of changes
Checklist
pytest
was run with the--run-skipped
flag.