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

feat: Added PydanticPintQuantity as an option to enforce unit validation for fields #56

Merged
merged 6 commits into from
Jan 15, 2025

Conversation

pesap
Copy link
Collaborator

@pesap pesap commented Nov 18, 2024

This was the original idea we intended before introducing BaseQuantity. It is more simple, but in the same spirt that we add an annotation as to enforce the unit as follow:

from typing import Annotated
from pint import Quantity
from infrasys.component import Component
from infrasys.pint_quantities import PydanticPintQuantity

class ACBus(Component):
    voltage: Annotated[Quantity, PydanticPintQuantity("volts")]

I do not plan to retire the BaseQuantity from infrasys, but this could be a simplier solution for people that do not want to create a class that enforces unit validation.

Someone already released this as a package here: https://github.com/tylerh111/pydantic-pint. We do not need the full implementation or dependency since we already had something similar.

@pesap pesap self-assigned this Nov 18, 2024
@pesap pesap requested a review from AadilLatif November 18, 2024 23:47
@codecov-commenter
Copy link

codecov-commenter commented Nov 18, 2024

Codecov Report

Attention: Patch coverage is 96.19048% with 4 lines in your changes missing coverage. Please review.

Project coverage is 96.06%. Comparing base (420e7d0) to head (644a266).

Files with missing lines Patch % Lines
src/infrasys/pint_quantities.py 90.69% 4 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main      #56    +/-   ##
========================================
  Coverage   96.06%   96.06%            
========================================
  Files          34       36     +2     
  Lines        2871     2976   +105     
========================================
+ Hits         2758     2859   +101     
- Misses        113      117     +4     

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

@KapilDuwadi
Copy link
Collaborator

KapilDuwadi commented Nov 19, 2024

@pesap I like the general idea. Is there a way to make it easier to ensure value must be positive, nonnegative, nonpositive etc. ?

Currently we are defining positive resistance class like below. I was wondering how would we migrate this to your new implementation.

class PositiveResistance(Resistance):
    """Quantity representing power system resistance."""

    def __init__(self, value, units, **kwargs):
        assert value >= 0, f"Resistance ({value}, {units}) must be positive."

@pesap
Copy link
Collaborator Author

pesap commented Nov 19, 2024

@pesap I like the general idea. Is there a way to make it easier to ensure value must be positive, nonnegative, nonpositive etc. ?

Currently we are defining positive resistance class like below. I was wondering how would we migrate this to your new implementation.

class PositiveResistance(Resistance):
    """Quantity representing power system resistance."""

    def __init__(self, value, units, **kwargs):
        assert value >= 0, f"Resistance ({value}, {units}) must be positive."

Yes, @KapilDuwadi . You can compose constraints. Using your example it would look like this,

from typing import Annotated
from pint import Quantity
from pydantic import Field
from infrasys.component import Component
from infrasys.pint_quantities import PydanticPintQuantity

class ACBus(Component):
    resistance: Annotated[Quantity, PydanticPintQuantity("ohms"), Field(gt=0)]

src/infrasys/pint_quantities.py Outdated Show resolved Hide resolved
src/infrasys/pint_quantities.py Show resolved Hide resolved
src/infrasys/pint_quantities.py Outdated Show resolved Hide resolved
src/infrasys/pint_quantities.py Outdated Show resolved Hide resolved
src/infrasys/pint_quantities.py Outdated Show resolved Hide resolved
tests/test_pint_quantities.py Show resolved Hide resolved
ser_mode : {"str", "dict"}, optional
The mode for serializing the field. Can be one of:
- `"str"`: Serialize to a string representation of the quantity (default in JSON mode).
- `"dict"`: Serialize to a dictionary representation.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems convenient, but I'm not completely following. It is being set per instance. Would the user set it after construction?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea after construction. We can remove it if we do not need it. It mostly import for serialization. Can we replace some of our logic here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's discuss it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Shall we raise this to an issue?

src/infrasys/pint_quantities.py Outdated Show resolved Hide resolved
@pesap pesap requested a review from daniel-thom November 19, 2024 20:56
@KapilDuwadi
Copy link
Collaborator

@pesap I like the general idea. Is there a way to make it easier to ensure value must be positive, nonnegative, nonpositive etc. ?
Currently we are defining positive resistance class like below. I was wondering how would we migrate this to your new implementation.

class PositiveResistance(Resistance):
    """Quantity representing power system resistance."""

    def __init__(self, value, units, **kwargs):
        assert value >= 0, f"Resistance ({value}, {units}) must be positive."

Yes, @KapilDuwadi . You can compose constraints. Using your example it would look like this,

from typing import Annotated
from pint import Quantity
from pydantic import Field
from infrasys.component import Component
from infrasys.pint_quantities import PydanticPintQuantity

class ACBus(Component):
    resistance: Annotated[Quantity, PydanticPintQuantity("ohms"), Field(gt=0)]

Cool makes sense.

@pesap pesap merged commit 7d7fbbf into main Jan 15, 2025
6 checks passed
@pesap pesap deleted the ps/pint_quantities branch January 15, 2025 02:08
github-actions bot pushed a commit that referenced this pull request Jan 15, 2025
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.

4 participants