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

Reorganize a bit the structure of the wall files #935

Merged
merged 53 commits into from
Aug 22, 2023

Conversation

tulioricci
Copy link
Collaborator

@tulioricci tulioricci commented Jul 17, 2023

Some cosmetic changes and initial steps towards a more thorough refactor of the gas-wall structure (#930)

  • some renaming regarding the wall calculations that I believe to make more sense now
  • moved the classes that are used for heat conduction only from driver to wall_model
  • moved the mask_from_elements to utils
  • new fluid state encapsulation PorousFlowFluidState to uncouple the wall density from dv to the new wv, as well as existing wall dependent vars wdv to wv
  • new GasModel-like encapsulation PorousFlowModel that considers the wall thermophysical properties.
  • new transport class PorousWallTransport coupling gas and wall properties
  • everything that is used exclusively by the ablation_workshop example is inside that driver. Now the material files are cleaner and more "prediction targeted"

Questions for the review @MTCam:

After quick first pass, looks good to me, just need to have a more detailed look to check the rest of the boxes.

  • Is the scope and purpose of the PR clear?
    • The PR should have a description.
    • The PR should have a guide if needed (e.g., an ordering).
  • Is every top-level method and class documented? Are things that should be documented actually so?
  • Is the interface understandable? (I.e. can someone figure out what stuff does?) Is it well-defined?
  • Does the implementation do what the docstring claims?
  • Is everything that is implemented covered by tests?
  • Do you see any immediate risks or performance disadvantages with the design? Example: what do interface normals attach to?

@tulioricci tulioricci marked this pull request as ready for review July 25, 2023 16:42
@tulioricci tulioricci requested review from majosm and MTCam July 25, 2023 16:42
@tulioricci tulioricci requested a review from majosm August 17, 2023 22:13
examples/ablation-workshop-mpi.py Outdated Show resolved Hide resolved
examples/ablation-workshop-mpi.py Outdated Show resolved Hide resolved
examples/ablation-workshop-mpi.py Outdated Show resolved Hide resolved
examples/ablation-workshop-mpi.py Outdated Show resolved Hide resolved
mirgecom/gas_model.py Show resolved Hide resolved
mirgecom/gas_model.py Outdated Show resolved Hide resolved
mirgecom/gas_model.py Outdated Show resolved Hide resolved
mirgecom/multiphysics/__init__.py Outdated Show resolved Hide resolved
mirgecom/transport.py Outdated Show resolved Hide resolved
mirgecom/wall_model.py Outdated Show resolved Hide resolved
@majosm
Copy link
Collaborator

majosm commented Aug 18, 2023

I forgot to mention: I have an idea to simplify make_fluid_state a little. I'll give it a shot, and if it pans out I'll make a PR (I won't let that hold up this PR though).

@tulioricci
Copy link
Collaborator Author

Hi @majosm, I think I covered all the points you raised. I couldn't reply directly wrt the eos and wall_eos in the transport variables, but with the latest changes, now it has the same structure of pure gas. I don't know if it is worth to remove one argument but add the classes in the init, as you suggested. It doesn't look like a significant improvement.

Also, note that I renamed wall_model in PorousFlowModel to wall_eos to keep consistency with the gas nomenclature. Hopefully this also helps makes things more clear.

mirgecom/eos.py Outdated
@@ -341,7 +342,7 @@ def heat_capacity_cv(self, cv: Optional[ConservedVars] = None, temperature=None)
"""
return self._gas_const / (self._gamma - 1)

def gas_const(self, cv: Optional[ConservedVars] = None):
def gas_const(self, cv: Optional[ConservedVars] = None, temperature=None):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def gas_const(self, cv: Optional[ConservedVars] = None, temperature=None):
def gas_const(self, cv: Optional[ConservedVars] = None, temperature: Optional[DOFArray] = None):

mirgecom/eos.py Outdated
Comment on lines 666 to 667
def gas_const(self, cv: ConservedVars, # type: ignore[override]
temperature=None):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def gas_const(self, cv: ConservedVars, # type: ignore[override]
temperature=None):
def gas_const(self, cv: ConservedVars, # type: ignore[override]
temperature: Optional[DOFArray] = None):

Comment on lines +51 to +52
# TODO per MTC review, can we generalize the oxidation model?
# should we keep this in the driver?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tbh, I do not see any real harm in internalizing some of our use-specific constructs. If it makes it easier for us (and others) to use, then we should. Is that the question, or did I miss the mark?

Copy link
Collaborator Author

@tulioricci tulioricci Aug 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original comment was here: #875 (comment)

Just keeping in the code as a reminder for the coupling PR which will effectively use the models. I suggest we postpone the discussion on this.

Copy link
Member

@MTCam MTCam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks like a net positive change to me. It is hard to tell what all the implications are, and the unintended effects. We'll just have to use it and find out, heh.

Copy link
Collaborator

@majosm majosm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few more suggestions, otherwise looks good. 👍

doc/mathematical/composite_material.rst Outdated Show resolved Hide resolved
examples/ablation-workshop.py Outdated Show resolved Hide resolved
examples/ablation-workshop.py Outdated Show resolved Hide resolved
mirgecom/wall_model.py Outdated Show resolved Hide resolved
mirgecom/wall_model.py Outdated Show resolved Hide resolved
mirgecom/wall_model.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@majosm majosm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good on my end.

@tulioricci tulioricci merged commit 294d5a6 into main Aug 22, 2023
@tulioricci tulioricci deleted the tulio/reorganize_phenolics branch August 22, 2023 15:11
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

Successfully merging this pull request may close these issues.

3 participants