Skip to content

FF till May 25 #6

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

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

FF till May 25 #6

wants to merge 15 commits into from

Conversation

alongd
Copy link
Member

@alongd alongd commented May 9, 2025

Fast-forward this repo to match the relevant development done in RMG in the last year.

New commits were made (I tried to keep the original commit messages and committer identity, but it was too complex for me to transfer those into another repo)

@alongd alongd requested a review from mjohnson541 May 9, 2025 22:35
Copy link
Contributor

@mjohnson541 mjohnson541 left a comment

Choose a reason for hiding this comment

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

For future reference, and figuring out the most efficient way to do these updates, how did you go about making this PR?

I skimmed it as best as I could. I noticed some functions that import coolprop being uncommented in data and some cantera imports being added in kinetics. How much testing has been done with this?

# """
# Returns the critical temperature of solvent from CoolProp
# """
# if self.name_in_coolprop is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like these are getting uncommented. I think these are commented to avoid requiring coolprop.

Copy link
Contributor

Choose a reason for hiding this comment

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

A number of cases in this file.

BlowersMaselRate(A, b, Ea, W) where A is in units of m^3/kmol/s,
b is dimensionless, and Ea and W are in J/kmol
"""
import cantera as ct
Copy link
Contributor

Choose a reason for hiding this comment

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

This and the many other cantera imports in this commit will cause an error.

@alongd
Copy link
Member Author

alongd commented May 10, 2025

Hi, thanks for the quick response. I started with this conversation:
https://chatgpt.com/share/681f6ac0-7994-8000-824f-49a047540604 (chat suggested git subtree split which was new to me)
But that didn't work as expected. I eventually did it in a less efficient way of copying the files per folder from RMG-Py onto a new branch in this repo, and manually cleaning (staging or discarding) the diff using GitKraken.
I still did not test this new branch... (I hope to get to it, no rush merging it)

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.

2 participants