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

Removed kinetic group additivity methods are still referenced in codebase #2605

Closed
jonwzheng opened this issue Feb 6, 2024 · 2 comments
Closed
Labels
bug bug which will never be closed by the actions bot Status: Discussion Needed Discussion needed for further progress

Comments

@jonwzheng
Copy link
Contributor

jonwzheng commented Feb 6, 2024

Bug Description

A commit from last August (9f293be) removed various deprecated code, including functions in rmgpy/data/kinetics/groups and rmgpy/data/kinetics/family related to computing rate coefficients using group additivity methods. Specifically, the method named estimate_kinetics_using_group_additivity in both of those files.

However, references to these methods are still available in the codebase. For example, in rmgpy/data/kinetics/family: get_kinetics_for_template includes the option to use group additivity methods. This template method is called in get_kinetics and uses group additivity if kinetics_estimator is not provided, or if group_additivity is provided as the estimator.

In rmgpy/rmg/main.py, the docstring mentions that group_additivity is a valid kinetic estimator method even though this does not seem to be the case anymore. And rmgpy/rmg/model.py considers the case if kinetics_estimator is group additivity.

Expected Behavior

Code should run without calling a removed method that leads to an AttributeError.

Additional Context

We could either revert these changes, or we could commit to removing all references to group additivity. Previous discussion (see #975 (comment) and codebase pre-commit) indicated that group additivity should be deprecated for kinetics, however it has been available and a default method on the RMG Website. It might be helpful to figure out if the group additivity methods are so bad that we should just remove them altogether, or if they might be potentially useful for users on the website.

@JacksonBurns
Copy link
Contributor

I think Bill would want these to stay - I'm pretty sure he had us use RMG website to do group additivity estimates during his kinetics class.

@JacksonBurns JacksonBurns pinned this issue Feb 8, 2024
@JacksonBurns JacksonBurns added Status: Discussion Needed Discussion needed for further progress bug bug which will never be closed by the actions bot labels Feb 8, 2024
@jonwzheng
Copy link
Contributor Author

Addressed by #2612

@bjkreitz bjkreitz unpinned this issue Feb 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug bug which will never be closed by the actions bot Status: Discussion Needed Discussion needed for further progress
Projects
None yet
Development

No branches or pull requests

2 participants