Conversation
…me as pe`. This avoids circular import issues when pyenzyme is used in 3rd party libraries
…ts function to organize measurement data by conditions. Update get_all_parameters to use Parameter directly. Added `group_measurements` method. Allowing to assign measurement.group_id based on uniqueness of initial conditions of MeasurementData.
Introduced a new test suite in `test_group_measurements.py` to validate the functionality of `_get_measurement_conditions` and `group_measurements` functions. The tests cover various scenarios including prepared and initial values, precedence rules, and grouping behavior based on identical and differing conditions. This enhances the test coverage for measurement data handling in the project.
…d tolerance-based grouping
Updated the `group_measurements` function to allow selection of concentration attributes ('initial' or 'prepared') and introduced a tolerance parameter for numerical comparisons when grouping measurements. Added helper functions for matching conditions within tolerance and checking value matches.
|
Enhances From my side, this PR is completed. |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a measurement grouping feature that assigns group IDs to measurements with identical initial conditions, facilitating analysis of related experimental data. Additionally, it refactors internal imports to use explicit submodule imports instead of top-level pyenzyme imports to prevent circular import issues.
- Adds a new
group_measurements()function with tolerance-based matching capabilities - Refactors imports throughout the codebase to use specific submodule imports
- Updates version number from 2.0.0 to 2.0.1
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_group_measurements.py | Comprehensive test suite for the new grouping functionality |
| pyenzyme/tools.py | Implementation of group_measurements and supporting functions |
| pyenzyme/init.py | Exports the new group_measurements function |
| pyproject.toml | Version bump to 2.0.1 |
| pyenzyme/suite.py | Import refactoring for EnzymeMLDocument and EnzymeMLHandler |
| pyenzyme/sbml/versions/v2.py | Import refactoring for DataTypes and Measurement |
| pyenzyme/sbml/versions/v1.py | Import refactoring for DataTypes and Measurement |
| pyenzyme/sbml/validation.py | Import refactoring for various EnzymeML types |
| pyenzyme/sbml/serializer.py | Import order adjustment |
| pyenzyme/petab/io.py | Import order adjustment |
pyenzyme/sbml/validation.py
Outdated
| *tools.extract(obj=doc, target=pe.MeasurementData), | ||
| *tools.extract(obj=doc, target=MeasurementData), | ||
| ] | ||
| print(f"Found {len(mandatory_unit_objects)} mandatory unit objects.") |
There was a problem hiding this comment.
Debug print statement should be removed before production. Use logger.debug() instead if debug information is needed.
| print(f"Found {len(mandatory_unit_objects)} mandatory unit objects.") | |
| logger.debug(f"Found {len(mandatory_unit_objects)} mandatory unit objects.") |
There was a problem hiding this comment.
You could use the global logger to keep the logging consistent, but I am also fine with using print or rich (for some color and formatting)
pyenzyme/tools.py
Outdated
| attribute: Literal["initial", "prepared"] = "initial", | ||
| ) -> dict: | ||
| """ | ||
| Extract measurement conditions as a dictionary, inclding pH and temperature. |
There was a problem hiding this comment.
Typo in docstring: 'inclding' should be 'including'.
| Extract measurement conditions as a dictionary, inclding pH and temperature. | |
| Extract measurement conditions as a dictionary, including pH and temperature. |
JR-1991
left a comment
JR-1991
left a comment
There was a problem hiding this comment.
Some minor comments, overall LGTM
pyenzyme/tools.py
Outdated
| list[Parameter]: A list of all parameters in the EnzymeMLDocument. | ||
| """ | ||
| return find_unique(enzmldoc, target=pe.Parameter) | ||
| return find_unique(enzmldoc, target=Parameter) |
There was a problem hiding this comment.
You can use the parameter array directly. Since the last version, we’ve consolidated all parameters into enzmldoc.parameters.
pyenzyme/tools.py
Outdated
|
|
||
| # Add species conditions using the specified attribute | ||
| for species_data in measurement.species_data: | ||
| if attribute == "initial": |
There was a problem hiding this comment.
You can use getattr to retrieve any of these without needing to branch, but this is fine.
…plifying measurement condition extraction. Updated import statements and fixed a typo in the docstring for _get_measurement_conditions.
…function and directly access parameters from the document. Updated import statements accordingly.
…tion, directly accessing parameters from the document instead. Updated import statements accordingly.
…DME badge to reflect current PyPI version.
This PR introduces a convenience method
pe.group_measurements()that assigns a group_id to allMeasuremententries within anEnzymeMLDocument. The method compares allMeasurementDataassociated with each species and groups those sharing identical initial conditions under the samegroup_id.Additionally, this PR refactors internal imports by replacing top-level import pyenzyme statements with explicit submodule imports. This change mitigates circular import issues, particularly when pyenzyme is used as a dependency in third-party packages.
This change is