Skip to content

Refactor SimpolPhaseTransfer to use composition instead of inheritance#494

Merged
K20shores merged 6 commits intomainfrom
copilot/fix-452
Jul 25, 2025
Merged

Refactor SimpolPhaseTransfer to use composition instead of inheritance#494
K20shores merged 6 commits intomainfrom
copilot/fix-452

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Jul 25, 2025

This PR refactors the SimpolPhaseTransfer class in musica.mechanism_configuration to use composition instead of inheritance from _SimpolPhaseTransfer, following the established pattern used in the Arrhenius class.

Changes Made

Core Refactoring

  • Removed inheritance: SimpolPhaseTransfer no longer inherits from _SimpolPhaseTransfer
  • Added composition: Uses self._instance = _SimpolPhaseTransfer() to hold the internal C++ instance
  • Property delegation: All attributes now use @property decorators to delegate to self._instance

Attribute Handling

  • Type conversion: gas_phase_species and aerosol_phase_species properties convert between Python Species/(float, Species) tuples and C++ _ReactionComponent objects
  • Phase handling: gas_phase and aerosol_phase accept both Phase objects and strings, converting to string names for the C++ instance
  • Validation: B parameter is validated as a list of exactly 4 elements on assignment

Serialization Updates

  • Instance method: Converted serialize() from static method to instance method using only Python-visible data
  • Static compatibility: Added serialize_static() method for backward compatibility with C++ _SimpolPhaseTransfer objects
  • Mechanism configuration: Updated mechanism_configuration.py to handle both C++ and Python instances separately

Public Interface

  • Type property: Added type property returning ReactionType.SimpolPhaseTransfer
  • API preserved: Public interface remains unchanged - existing code continues to work
  • Python-native types: Public interface exposes only Python-native types without direct C++ inheritance

Example Usage

import musica.mechanism_configuration as mc

# Create species and phases
ethanol = mc.Species(name="ethanol")
ethanol_aq = mc.Species(name="ethanol_aq")
gas = mc.Phase(name="gas")
aerosol = mc.Phase(name="aerosol")

# Usage remains the same
simpol = mc.SimpolPhaseTransfer(
    name="my simpol",
    gas_phase=gas,
    gas_phase_species=ethanol,
    aerosol_phase=aerosol,
    aerosol_phase_species=ethanol_aq,
    B=[-1.97e03, 2.91e00, 1.96e-03, -4.96e-01],
    other_properties={"test": "value"}
)

# Properties work transparently
assert simpol.name == "my simpol"
assert simpol.gas_phase == "gas"
assert len(simpol.B) == 4

# Serialization works as instance method
result = simpol.serialize()

Validation

All existing tests continue to pass as the public API remains unchanged. The refactoring follows the same pattern established in similar classes like Arrhenius.

Fixes #452.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI changed the title [WIP] Refactor SimpolPhaseTransfer Refactor SimpolPhaseTransfer to use composition instead of inheritance Jul 25, 2025
Copilot AI requested a review from K20shores July 25, 2025 13:09
"""Get the reaction type."""
return ReactionType.SimpolPhaseTransfer

def _create_serialize_dict(self, instance) -> Dict:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Remove _create_serialize_dict and place the dictionary directly in serialize

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Removed the _create_serialize_dict helper method and inlined the dictionary creation directly in both serialize() and serialize_static() methods. (7aa8eab)

Copilot AI requested a review from K20shores July 25, 2025 13:21
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jul 25, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.04%. Comparing base (000ddb2) to head (95e34e7).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #494      +/-   ##
==========================================
+ Coverage   84.85%   85.04%   +0.18%     
==========================================
  Files          49       49              
  Lines        4815     4873      +58     
==========================================
+ Hits         4086     4144      +58     
  Misses        729      729              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.


@staticmethod
def serialize(instance) -> Dict:
def serialize_static(instance) -> Dict:
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Create a local copy to call serialize so that the dictionary doesn't need to be duplicated

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Modified serialize_static() to create a local SimpolPhaseTransfer object and call its serialize() method, eliminating the dictionary duplication. (28e352c)

Copilot AI requested a review from K20shores July 25, 2025 17:04
@K20shores K20shores marked this pull request as ready for review July 25, 2025 18:41
aerosol_phase_species (Union[Species, Tuple[float, Species]]): The aerosol phase species involved in the reaction.
B (List[float]): The B parameters [unitless].
other_properties (Dict[str, Any]): A dictionary of other properties of the simplified phase transfer reaction rate constant.
unknown_properties (Dict[str, Any]): A dictionary of other properties of the simplified phase transfer reaction rate constant.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This should be labelled other_properties as above

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed the docstring parameter name to match the actual parameter name other_properties. (95e34e7)

Copilot AI requested a review from mattldawson July 25, 2025 18:57
@K20shores K20shores merged commit 1338b4e into main Jul 25, 2025
69 checks passed
@K20shores K20shores deleted the copilot/fix-452 branch July 25, 2025 20:14
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.

Refactor SimpolPhaseTransfer

5 participants