Merging Anna's changes from 'Pdep spin 39' to the main RMG branch #2837

akshatzalte · 2025-07-23T18:21:49Z

Motivation or Problem

A clear and concise description of what what you're trying to fix or improve. Please reference any issues that this addresses.

Description of Changes

A clear and concise description of what what you've changed or added.

Testing

A clear and concise description of testing that you've done or plan to do.

Reviewer Tips

Suggestions for verifying that this PR works or other notes for the reviewer.

Copilot

Pull Request Overview

This pull request appears to merge Anna's changes from a branch called 'Pdep spin 39' to the main RMG branch. The PR includes improvements to Julia integration, logging configuration, spin conservation checking in kinetic families, and various code cleanup tasks.

Replaced global Julia import with lazy loading to delay Julia initialization until needed
Added spin conservation checking in kinetic reaction families with configurable exceptions
Modernized logging configuration to use Python 3.8+ force=True parameter

Reviewed Changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
scripts/checkModels.py	Modernized logging configuration using force=True
rmgpy/rmg/reactionmechanismsimulator_reactors.py	Implemented lazy Julia loading with LazyMain class
rmgpy/rmg/pdep.py	Removed energy correction calculations from network updates
rmgpy/rmg/model.py	Removed NO_JULIA references and added irreversible reaction check
rmgpy/rmg/main.py	Cleaned up NO_JULIA import
rmgpy/rmg/input.py	Removed NO_JULIA error handling
rmgpy/pdep/sls.py	Implemented lazy Julia loading similar to reactors module
rmgpy/molecule/fragment_utils.py	Fixed function call prefixes from FragList to direct function calls
rmgpy/data/kinetics/family.py	Added spin conservation checking with allowed violations list
rmgpy/init.py	Minor whitespace cleanup
install_rms.sh	Enhanced RMS installation script with better environment handling
documentation/source/users/rmg/installation/index.rst	Updated installation documentation structure
documentation/source/users/rmg/installation/anacondaDeveloper.rst	Updated example paths in documentation
documentation/source/users/rmg/faq.rst	Simplified installation reference
Makefile	Added new RMS reactor examples (eg8, eg9, eg10)
Dockerfile	Updated Docker configuration with Julia 1.10 and improved build process
.github/workflows/CI.yml	Enhanced CI workflow with better error handling and Docker build improvements

Copilot · 2025-07-23T18:33:47Z

rmgpy/rmg/pdep.py

            rxn.fix_barrier_height(force_positive=True)
            if rxn.network_kinetics is None:
-                E0 = sum([spec.conformer.E0.value_si for spec in rxn.reactants]) + rxn.kinetics.Ea.value_si + energy_correction
+                E0 = sum([spec.conformer.E0.value_si for spec in rxn.reactants]) + rxn.kinetics.Ea.value_si 


Trailing whitespace should be removed from this line.

Suggested change

E0 = sum([spec.conformer.E0.value_si for spec in rxn.reactants]) + rxn.kinetics.Ea.value_si

E0 = sum([spec.conformer.E0.value_si for spec in rxn.reactants]) + rxn.kinetics.Ea.value_si

Copilot · 2025-07-23T18:33:47Z

rmgpy/rmg/pdep.py

+                E0 = sum([spec.conformer.E0.value_si for spec in rxn.reactants]) + rxn.kinetics.Ea.value_si 
            else:
-                E0 = sum([spec.conformer.E0.value_si for spec in rxn.reactants]) + rxn.network_kinetics.Ea.value_si + energy_correction
+                E0 = sum([spec.conformer.E0.value_si for spec in rxn.reactants]) + rxn.network_kinetics.Ea.value_si 


Trailing whitespace should be removed from this line.

Copilot · 2025-07-23T18:33:48Z

rmgpy/rmg/model.py

        label = orilabel
        i = 2
-        if self.edge.phase_system:  # None when RMS not installed
+        if self.edge.phase_system:  # !!! Not maintained when operating with require_rms=False?


This comment with '!!!' appears to indicate incomplete implementation or uncertainty. Consider clarifying the behavior or removing the comment if the code is correct.

Suggested change

if self.edge.phase_system: # !!! Not maintained when operating with require_rms=False?

if self.edge.phase_system: # Ensure unique species labels only when phase_system is defined.

Copilot · 2025-07-23T18:33:48Z

rmgpy/rmg/model.py

        if check_cut:
            try:
                mols = molecule.cut_molecule(cut_through=False)
            except AttributeError:


[nitpick] The comment explaining why this conversion is needed was removed. Consider restoring it as it provides important context for this non-obvious operation.

Suggested change

except AttributeError:

except AttributeError:

# Convert the Molecule to a Fragment using its adjacency list.

# This is necessary because the `cut_molecule` method is only available for Fragment objects.

Copilot · 2025-07-23T18:33:48Z

rmgpy/data/kinetics/family.py

+        if reactants_combined_spin.intersection(products_combined_spin) != set([]):
+            return True
+        else:
+            logging.debug(f"Reactants combined spin is {reactants_combined_spin}, but the products combined spin is {products_combined_spin}")


[nitpick] The log message uses 'but' which implies a problem, but this might be called in valid cases. Consider using more neutral language like 'and' instead of 'but'.

Suggested change

logging.debug(f"Reactants combined spin is {reactants_combined_spin}, but the products combined spin is {products_combined_spin}")

logging.debug(f"Reactants combined spin is {reactants_combined_spin}, and the products combined spin is {products_combined_spin}")

Copilot · 2025-07-23T18:33:49Z

rmgpy/data/kinetics/family.py

+        # get the combined spin for reactants and products
+        reactants_combined_spin, products_combined_spin = self.calculate_combined_spin()
+        # check if there are any matches for combined spin between reactants and products
+        if reactants_combined_spin.intersection(products_combined_spin) != set([]):


Use set() instead of set([]) for creating an empty set, or use the more Pythonic if reactants_combined_spin.intersection(products_combined_spin):.

Suggested change

if reactants_combined_spin.intersection(products_combined_spin) != set([]):

if reactants_combined_spin.intersection(products_combined_spin):

Copilot · 2025-07-23T18:33:49Z

rmgpy/data/kinetics/family.py

+        if len(self.reactants) == 1:
+            reactant_combined_spin = {self.reactants[0].multiplicity}
+        elif len(self.reactants) == 2:
+            reactant_spin_string =  "+".join(sorted([str(reactant.multiplicity) for reactant in self.reactants]))
+            reactant_combined_spin = allowed_spin[reactant_spin_string]
+        else:
+            return None
+        if len(self.products) == 1:
+            product_combined_spin = {self.products[0].multiplicity}
+        elif len(self.products) == 2:
+            product_spin_string = "+".join(sorted([str(product.multiplicity) for product in self.products]))
+            product_combined_spin = allowed_spin[product_spin_string]
+        else:
+            return None


This method returns None for reactions with more than 2 reactants/products, but the caller may not handle this case properly. Consider raising an exception or documenting this limitation clearly.

Suggested change

if len(self.reactants) == 1:

reactant_combined_spin = {self.reactants[0].multiplicity}

elif len(self.reactants) == 2:

reactant_spin_string = "+".join(sorted([str(reactant.multiplicity) for reactant in self.reactants]))

reactant_combined_spin = allowed_spin[reactant_spin_string]

else:

return None

if len(self.products) == 1:

product_combined_spin = {self.products[0].multiplicity}

elif len(self.products) == 2:

product_spin_string = "+".join(sorted([str(product.multiplicity) for product in self.products]))

product_combined_spin = allowed_spin[product_spin_string]

else:

return None

"""

Calculate the combined spin multiplicities for reactants and products.

This method supports reactions with up to two reactants and two products.

If there are more than two reactants or products, a ValueError is raised.

Returns:

tuple: A tuple containing sets of combined spin multiplicities for reactants and products.

Raises:

ValueError: If there are more than two reactants or products.

"""

if len(self.reactants) == 1:

reactant_combined_spin = {self.reactants[0].multiplicity}

elif len(self.reactants) == 2:

reactant_spin_string = "+".join(sorted([str(reactant.multiplicity) for reactant in self.reactants]))

reactant_combined_spin = allowed_spin[reactant_spin_string]

else:

raise ValueError("calculate_combined_spin only supports up to two reactants.")

if len(self.products) == 1:

product_combined_spin = {self.products[0].multiplicity}

elif len(self.products) == 2:

product_spin_string = "+".join(sorted([str(product.multiplicity) for product in self.products]))

product_combined_spin = allowed_spin[product_spin_string]

else:

raise ValueError("calculate_combined_spin only supports up to two products.")

Copilot · 2025-07-23T18:33:49Z

rmgpy/data/kinetics/family.py

+        if len(self.reactants) == 1:
+            reactant_combined_spin = {self.reactants[0].multiplicity}
+        elif len(self.reactants) == 2:
+            reactant_spin_string =  "+".join(sorted([str(reactant.multiplicity) for reactant in self.reactants]))
+            reactant_combined_spin = allowed_spin[reactant_spin_string]
+        else:
+            return None
+        if len(self.products) == 1:
+            product_combined_spin = {self.products[0].multiplicity}
+        elif len(self.products) == 2:
+            product_spin_string = "+".join(sorted([str(product.multiplicity) for product in self.products]))
+            product_combined_spin = allowed_spin[product_spin_string]
+        else:
+            return None


This method returns None for reactions with more than 2 reactants/products, but the caller may not handle this case properly. Consider raising an exception or documenting this limitation clearly.

Suggested change

if len(self.reactants) == 1:

reactant_combined_spin = {self.reactants[0].multiplicity}

elif len(self.reactants) == 2:

reactant_spin_string = "+".join(sorted([str(reactant.multiplicity) for reactant in self.reactants]))

reactant_combined_spin = allowed_spin[reactant_spin_string]

else:

return None

if len(self.products) == 1:

product_combined_spin = {self.products[0].multiplicity}

elif len(self.products) == 2:

product_spin_string = "+".join(sorted([str(product.multiplicity) for product in self.products]))

product_combined_spin = allowed_spin[product_spin_string]

else:

return None

"""

Calculate the combined spin states for reactants and products.

This method supports reactions with up to two reactants and two products.

If there are more than two reactants or products, a ValueError is raised.

Returns:

tuple: A tuple containing the combined spin states for reactants and products.

Raises:

ValueError: If there are more than two reactants or products.

"""

if len(self.reactants) == 1:

reactant_combined_spin = {self.reactants[0].multiplicity}

elif len(self.reactants) == 2:

reactant_spin_string = "+".join(sorted([str(reactant.multiplicity) for reactant in self.reactants]))

reactant_combined_spin = allowed_spin[reactant_spin_string]

else:

raise ValueError("calculate_combined_spin only supports up to two reactants.")

if len(self.products) == 1:

product_combined_spin = {self.products[0].multiplicity}

elif len(self.products) == 2:

product_spin_string = "+".join(sorted([str(product.multiplicity) for product in self.products]))

product_combined_spin = allowed_spin[product_spin_string]

else:

raise ValueError("calculate_combined_spin only supports up to two products.")

…o python 3.9

akshatzalte requested a review from Copilot July 23, 2025 18:32

Copilot AI reviewed Jul 23, 2025

View reviewed changes

donerancl added 6 commits July 23, 2025 14:57

fixed functions in fragment_utils.py

b972628

moved ammonia project PR changes to new branch after update of main t…

650e116

…o python 3.9

pdep spin conservation updates

15cdb8c

debugging pdep

edc03c4

revert update_unimolecular_reaction_network

aac9a56

revrt def merge

960dfb4

akshatzalte force-pushed the pdep_spin_39 branch from 5ad9e1e to 960dfb4 Compare July 23, 2025 18:59

JacksonBurns mentioned this pull request Aug 26, 2025

Pdep spin conservation #2688

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Merging Anna's changes from 'Pdep spin 39' to the main RMG branch #2837

Merging Anna's changes from 'Pdep spin 39' to the main RMG branch #2837

akshatzalte commented Jul 23, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jul 23, 2025

Uh oh!

Copilot AI Jul 23, 2025

Uh oh!

Copilot AI Jul 23, 2025

Uh oh!

Copilot AI Jul 23, 2025

Uh oh!

Copilot AI Jul 23, 2025

Uh oh!

Copilot AI Jul 23, 2025

Uh oh!

Copilot AI Jul 23, 2025

Uh oh!

Copilot AI Jul 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	E0 = sum([spec.conformer.E0.value_si for spec in rxn.reactants]) + rxn.kinetics.Ea.value_si
	E0 = sum([spec.conformer.E0.value_si for spec in rxn.reactants]) + rxn.kinetics.Ea.value_si

	if self.edge.phase_system: # !!! Not maintained when operating with require_rms=False?
	if self.edge.phase_system: # Ensure unique species labels only when phase_system is defined.

	logging.debug(f"Reactants combined spin is {reactants_combined_spin}, but the products combined spin is {products_combined_spin}")
	logging.debug(f"Reactants combined spin is {reactants_combined_spin}, and the products combined spin is {products_combined_spin}")

	if reactants_combined_spin.intersection(products_combined_spin) != set([]):
	if reactants_combined_spin.intersection(products_combined_spin):

Merging Anna's changes from 'Pdep spin 39' to the main RMG branch #2837

Are you sure you want to change the base?

Merging Anna's changes from 'Pdep spin 39' to the main RMG branch #2837

Conversation

akshatzalte commented Jul 23, 2025

Motivation or Problem

Description of Changes

Testing

Reviewer Tips

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 23, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants