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

Update cantera to version 3.0.0 #989

Merged
merged 22 commits into from
Feb 6, 2024
Merged

Update cantera to version 3.0.0 #989

merged 22 commits into from
Feb 6, 2024

Conversation

tulioricci
Copy link
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
test/test_eos.py Outdated
@@ -302,7 +300,7 @@ def inf_norm(x):
pyro_t = pyro_mechanism.get_temperature(pyro_e, tin, yin)
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
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
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
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
Collaborator Author

Choose a reason for hiding this comment

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

Removed. 👍

Comment on lines -167 to -170
if use_overintegration:
quadrature_tag = DISCR_TAG_QUAD
else:
quadrature_tag = None
Copy link
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
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
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
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
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.

examples/autoignition.py Outdated Show resolved Hide resolved
@tulioricci tulioricci merged commit e92d608 into main Feb 6, 2024
11 of 13 checks passed
@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