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 the CEPTR usage doc slightly. #499

Merged
merged 3 commits into from
May 21, 2024

Conversation

EnnaDelfen
Copy link
Contributor

Hi Marc, here's the PR with a couple modifs to the CEPTR documentation.
I reorganized the section a little for it to follow the actual steps performed (generate a yaml first, then generate the single mechanisms file, then maybe to the batch generation).
I also made sure the arguments given in the code snippets are the same as in the text because it was confusing me at times.
I've tried clarifying the commands for generating a single QSS YAML file and then I wanted to explain the steps for generating a single QSS Pele mechanism file BEFORE the batch generation is mentioned - but then I found that there's an entire other section dedicated to that so I let the reference to it and simply moved the QSS section before the batched section.
Finally I warned users that one may need a certain Python version/Cantera to be installed.
Overall, very little but I think it's a tad improved.

@EnnaDelfen EnnaDelfen requested a review from marchdf May 15, 2024 22:20
@marchdf marchdf requested review from malihass and baperry2 May 16, 2024 13:55
Comment on lines 70 to 79
.. note:: It is possible that using this option will require for you to have a valid Cantera installed somewhere. Again, we strongly suggest using a `conda <https://conda.io/projects/conda/en/latest/user-guide/getting-started.html>`_ environment to install all required package. A simple ceptr environment can be generated using the following yaml script::

$ name: ceptr
$ channels:
$ - conda-forge
$ dependencies:
$ - python=3.10
$ - cantera
$ - cython
$ - numpy
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems to repeat cantera and numpy dependencies compared with pyproject.toml
I'd prefer if we keep the conda install dependencies to a minimum:

Could you please try to see if creating you conda env with conda create --name ceptr python=3.10 poetry before calling convert.sh does the trick?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As always, I can't seem to reproduce the errors that I had now that I've tried/installed all these various environments. I know for a fact that simply invoking the convert.sh from the Mechanism directory directly (LiDryer) gave me , first a python and then a cantera error. Which I solved by including cantera in the ceptr env; and which was the entire point of my modifying the instructions actually ... if we remove that then, yes I've slightly improved the doc but I'm not addressing the main issue I encountered which is a lack of Cantera that somehow wasn't automatically taken care of.

Copy link
Collaborator

@malihass malihass May 18, 2024

Choose a reason for hiding this comment

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

Clearly the solution you provided is working, but it is not ideal since pyproject.toml is supposed to already take care of the cantera dependency.
I just tried what I suggested on kestrel on a fresh conda env and it seems to work, but it'd be good if we can confirm that it does take care of this issue.
@drummerdoc you mentioned you had the same problem as @EnnaDelfen , could you maybe please try what I mentioned?

Copy link
Contributor Author

@EnnaDelfen EnnaDelfen May 19, 2024

Choose a reason for hiding this comment

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

I've tried everything on another (older) mac system and ran into issues obtaining cantera via the poetry channel. This time the issue was a bit different in that the poetry update clearly indicated that cantera was not found and thus not installed. So the conda create --name ceptr python=3.10 poetry alone doesn't fix the issue. Again the only fix I could find was to use the conda-forge channel to create a conda env that includes cantera. I've explained everything to Malik in private. The source of the issue may not be completely identified but the proposed note (which I have simplified to python 3.10 and cantera, no need for numpy or cython) did fix all my issues so far. But you can go ahead and ignore it if you prefer it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for checking all that!

@@ -19,13 +19,27 @@ To install CEPTR dependencies::
$ cd ${PELE_PHYSICS_HOME}/Support/ceptr
$ poetry update

.. note:: Note that you will need to use a Python version >=3.10 and <3.12. If a compatible version exists in your system then poetry will try to find and use it. Otherwise, think about using a `conda <https://conda.io/projects/conda/en/latest/user-guide/getting-started.html>`_ environment to manage packages and their dependencies without tampering with your system.
Copy link
Contributor

Choose a reason for hiding this comment

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

How about saying to check Support/ceptr/pyproject.toml for the latest supported python versions in case they get updated and we forget to modify this text in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@marchdf marchdf merged commit 4157d7a into AMReX-Combustion:development May 21, 2024
10 checks passed
SreejithNREL pushed a commit that referenced this pull request May 27, 2024
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.

4 participants