Skip to content

Update cantera to version 3.0.0#989

Merged
tulioricci merged 22 commits into
mainfrom
use_cantera30
Feb 6, 2024
Merged

Update cantera to version 3.0.0#989
tulioricci merged 22 commits into
mainfrom
use_cantera30

Conversation

@tulioricci
Copy link
Copy Markdown
Collaborator

@tulioricci tulioricci commented Dec 22, 2023

Questions for the review:

  • 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?

Base automatically changed from use_cantera26 to main December 22, 2023 16:58
@tulioricci tulioricci added the nomerge Not meant for merging label Dec 22, 2023
@tulioricci tulioricci removed the nomerge Not meant for merging label Jan 7, 2024
@tulioricci tulioricci marked this pull request as ready for review January 31, 2024 17:49
@tulioricci tulioricci requested a review from MTCam January 31, 2024 17:49
Comment thread test/test_eos.py Outdated
pyro_p = pyro_mechanism.get_pressure(pyro_m, pyro_t, yin)
pyro_h = pyro_mechanism.get_mixture_enthalpy_mass(pyro_t, yin)
pyro_s = pyro_mechanism.get_mixture_entropy_mass(pyro_p, pyro_t, yin)
# pyro_s = pyro_mechanism.get_mixture_entropy_mass(pyro_p, pyro_t, yin)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

OOC, how's come?

Copy link
Copy Markdown
Collaborator Author

@tulioricci tulioricci Feb 1, 2024

Choose a reason for hiding this comment

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

Per Pyrometheus review, we won't add entropy to its native functions. I am wondering if I should remove it completely or add local functions somewhere to keep the verification.

Copy link
Copy Markdown
Member

@MTCam MTCam Feb 1, 2024

Choose a reason for hiding this comment

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

If it is no longer in the base package and the corresponding test was removed there, then this is the right thing to do. I was just curious about the removal. (I'd lean on complete removal).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Removed. 👍

Comment thread examples/autoignition.py
Comment on lines -167 to -170
if use_overintegration:
quadrature_tag = DISCR_TAG_QUAD
else:
quadrature_tag = None
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why remove?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Removed because it is useless without spatial operators.

Copy link
Copy Markdown
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.

I am not sure I am comfortable removing the fluid stuff for this example. It is not wasted on me that it is largely non-participant in this example, but part of the example was showing how to have chem+fluid in the simulation.

@tulioricci
Copy link
Copy Markdown
Collaborator Author

I am not sure I am comfortable removing the fluid stuff for this example. It is not wasted on me that it is largely non-participant in this example, but part of the example was showing how to have chem+fluid in the simulation.

My plan is to make autoignition for chemistry only (faster execution) and to add a 1D flame driver to verify spatial dependence + mixture transport models. I've been using both these cases for verification/debugging of chemistry in mirge-com. To avoid excess/superfluous examples, I want to remove mixture.py. See #995

However, I can keep the spatial terms in autoignition. No big deal.

@MTCam
Copy link
Copy Markdown
Member

MTCam commented Feb 1, 2024

My plan is to make autoignition for chemistry only (faster execution) and to add a 1D flame driver to verify spatial dependence + mixture transport models. I've been using both these cases for verification/debugging of chemistry in mirge-com. To avoid excess/superfluous examples, I want to remove mixture.py. See #995

However, I can keep the spatial terms in autoignition. No big deal.

I'm not opposed. Hearing your plan makes it seem like a good idea.

Comment thread examples/autoignition.py Outdated
@tulioricci tulioricci merged commit e92d608 into main Feb 6, 2024
@tulioricci tulioricci deleted the use_cantera30 branch February 6, 2024 19: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.

Need to keep cantera in version 2.6.0 (for now)

2 participants