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

Units in models and parsers #84

Open
NicolasGensollen opened this issue May 15, 2018 · 10 comments
Open

Units in models and parsers #84

NicolasGensollen opened this issue May 15, 2018 · 10 comments
Assignees
Labels
Beta Beta release milestone discussion enhancement New feature or request High Priority
Milestone

Comments

@NicolasGensollen
Copy link
Member

A few unit related problems I noticed:

  • Unit conversions are currently managed in the parsers (most of the time hardcoded when setting the attribute value), which is hard to maintain.

Ex:

#From cyme reader
api_winding.nominal_voltage=float(settings['primarybasevoltage'])*10**3 #DiTTo in volt
  • Attribute units are not clear when looking at the DiTTo model. Sometimes there is some indication in the help but not always. In addition some names are misleading in terms of the unit assumed by DiTTo.

Ex:

#In KVA or VA???
normhkva = Float(help='Normal maximum kVA rating for H winding', default_value=None)

These are already introducing some bugs in the conversions and it will probably get worse as we add more parsers...

Having a more robust framework to handle units (like Pint maybe??) would be a nice addition in my opinion. Thoughts?

@kdheepak
Copy link
Member

kdheepak commented Aug 1, 2018

Unfortunately, as a general statement, Python doesn't have a good solution for units. Though, for our specific use case we may be able to find something that works. Pint may not play well with traitlets, I haven't tested it but from looking at the source code it seems like both Pint and traitlets make heavy use of descriptors and subclass constraints (using __new__), and that may mean that they may not compose well with each other. I would first experiment with using callbacks or validation in traitlets to see if you can use that to solve your problem [1].

[1] https://traitlets.readthedocs.io/en/stable/api.html#callbacks-when-trait-attributes-change

@NicolasGensollen
Copy link
Member Author

@tarekelgindy and I had a discussion on this subject this morning. The following is a possible solution we came up with:

Change the models and add a unit and a unit_type keyword:

For example, in wire.py:

X = Float(help="x coordinate of the wire", default=0, unit="m", unit_type="distance")

In the readers, the attribute setting should be changed to:
read.py

#The input gives a x coordinate of 10 feet. The user doesn't have to worry about conversion
api_wire.set("X", 10, "ft") 

The set function has to be implemented in the base class. It probably has to check that X is a valid attribute of wire object, and that "ft" is a valid unit for this attribute (this is why we have a unit_type keyword which maps the type of unit to a set of authorized units: "distance": set("m","km","ft","kft","mi",...)). The set method then does the conversion from feet to meters using Pint or something else.
This way, if we decide to change the way X is represented in DiTTo, we simply change unit="m" by unit="kft" for example.

What are your thoughts on this @kdheepak? Do you think we could implement something like that?

@kdheepak
Copy link
Member

kdheepak commented Aug 7, 2018

It might work well. Why do we need unit_type?

@kdheepak
Copy link
Member

kdheepak commented Aug 7, 2018

I'll also need to prototype this or see a prototype and experiment to see if it works as intended.

@NicolasGensollen
Copy link
Member Author

We might need unit type to avoid stuffs like:

api_wire.set("X",10,"kva") #KVA is not a valid unit for length

There might be other ways to do that though...

@NicolasGensollen
Copy link
Member Author

I'm trying to see what I can do with that but I have troubles getting the attributes' kwargs in the DiTToHasTraits class where I am implementing the set function. I need to get the unit information defined in wire.X for example. I can get the values using _trait_values but not the other arguments (help, unit, unit_type...)

This is what I get when I print self.__dict__ in DiTToHasTraits.

'_trait_validators': {}, '_model': <ditto.store.Store(elements=0, models=1) object at 0x10c15e790>, '_trait_values': {'diameter': None, 'Y': None, 'insulation_thickness': 0.0, 'is_fuse': None, 'resistance': None, 'is_network_protector': 0, 'is_open': None, 'concentric_neutral_gmr': None, 'ampacity': None, 'is_switch': None, 'gmr': None, 'emergency_ampacity': None, 'concentric_neutral_diameter': None, 'concentric_neutral_resistance': None, 'phase': None, 'X': 10.0, 'response': None, 'interrupting_rating': None, 'drop': 0, 'nameclass': None, 'is_breaker': None, 'is_recloser': None}, '_cross_validation_lock': False, '_trait_notifiers': {}}

I tried, with no success, modifying the Float class to store this information:

class Float(T.Float, DiTToTraitType):
    def __init__(self,**kwargs):
         if "unit" in kwargs:
             self.unit=kwargs["unit"]
         super.__init__(self)

Since I've never really worked with traitlets before, I'm not sure how to do this. Any idea?
Hope that makes sense...

@kdheepak
Copy link
Member

kdheepak commented Aug 7, 2018

I'm able to do something like this:

import traitlets as T

class Float(T.Float, T.TraitType):
    def __init__(self, **kwargs):
         if "unit" in kwargs:
             self.unit=kwargs.pop("unit")
         super().__init__(**kwargs)

class Test(T.HasTraits):
    x = Float(unit="m")


t = Test(x=1)
print(t.x)
print(Test.x.unit)

but I still need to think about this more before we commit to doing it this way. Specifically, the set method is not ideal. And I still don't see a strong need for unit_type. That may be valuable for developers but I don't see it valuable for users.

@NicolasGensollen
Copy link
Member Author

Thanks for having a look @kdheepak , I'll try again then.
We might not need to expose unit_type to the user but I think we need to have a way to check that the units given by the user make sense.
I'm changing the priority of this since it is creating a lot of errors currently.

@kdheepak
Copy link
Member

kdheepak commented Aug 24, 2018

Here's a prototype of something that could work in DiTTo.

import traitlets as T

import pint

UNIT_REGISTRY = pint.UnitRegistry()

class DiTToHasTraits(T.HasTraits):
    
    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        self._units_registry = UNIT_REGISTRY
    
        self._units = {}
        
        for name in self.trait_names():
            trait = getattr(self.__class__, name)
            self._units[name] = trait.metadata["units"]
    

class Line(DiTToHasTraits):
    x = T.Float().tag(units="meter")
    y = T.Float().tag(units="meter")
    
    def compute_impedance_matrix(self, a, b, a_units=None, b_units=None):

        if a_units is None:
            a_units = self._units["x"] # Or some other default value
        if b_units is None:
            b_units = self._units["y"] # Or some other default value
        unitized_a = ( x * getattr(self._units_registry, a_units) ).to(self._units_registry.meter) # convert to unit that will be used internally, in this case meter.
        unitized_b = ( y * getattr(self._units_registry, b_units) ).to(self._units_registry.meter) # convert to unit that will be used internally, in this case meter.
        
        print(unitized_a, unitized_b)


l = Line(x=1, y=1)

l.compute_impedance_matrix(x=1, y=1, y_units="centimeter")

This prints out the following:

1 meter 0.01 meter

@NicolasGensollen
Copy link
Member Author

Steps to start integrating units:

  • Add units to each model attribute (see Add units to models #180)

  • Move Parser code to model classes

  • Update tests from test_parser.py

  • Write the set function set(attribute_name, attribute_value, unit) in Base class

  • Implement set_missing_attributes in the Base class and overwrite in the subclasses

To think

  • How to integrate this with the writers e.g. get function in the same way??

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Beta Beta release milestone discussion enhancement New feature or request High Priority
Projects
None yet
Development

No branches or pull requests

2 participants