[ENH] Add units for stored properties - #239
Merged
Merged
Conversation
There was a problem hiding this comment.
Pull Request Overview
Adds pint-based unit support for stored ORM properties and related tests
- Introduces
UnitMixinand a globalUnitRegistryinmodels.pyto enable<attribute>_uaccessors returningpint.Quantity - Updates element, isotope, ionic radius, ionization energy, phase transition, and scattering factor classes to mix in
UnitMixin - Adds unit tests in
tests/test_units.py, updates dependencies (pint) and CI (POETRY_VERSION), and a newtimeimportdev task
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_units.py | Added tests for _u-suffixed unit accessors |
| tasks.py | Added timeimport task to measure import performance |
| pyproject.toml | Added pint dependency |
| mendeleev/models.py | Introduced UnitRegistry, UnitMixin, and updated ORM classes |
| .github/workflows/main.yml | Bumped Poetry version in CI environment |
Comments suppressed due to low confidence (5)
.github/workflows/main.yml:4
- The
envsection should define a mapping rather than a list item; ensure correct indentation and remove any leading-so CI reads this variable.
POETRY_VERSION: 2.1.3
tasks.py:137
- [nitpick] The new
timeimporttask lacks a docstring. Consider adding one to describe its purpose and usage.
@task
tests/test_units.py:174
- Tests cover stored properties but omit computed unit properties such as
boiling_point_uandmelting_point_u. Consider adding tests for these_uaccessors.
# methods
mendeleev/models.py:1200
- Typo in
half_life_u: usingself.half_fileinstead ofself.half_lifewill raise an AttributeError. Change toself.half_life.
return self.half_file * ureg(self.half_life_unit)
mendeleev/models.py:88
- The code uses
enum.Enumbutenumis not imported. Addimport enumat the top of the file.
class ValueOrigin(enum.Enum):
lmmentel
enabled auto-merge (squash)
September 16, 2025 18:28
lmmentel
disabled auto-merge
September 16, 2025 18:39
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See discussion #162
TODO