Open NicolasGensollen opened 6 years ago
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
@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?
It might work well. Why do we need unit_type
?
I'll also need to prototype this or see a prototype and experiment to see if it works as intended.
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...
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...
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.
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.
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
Steps to start integrating units:
[ ] Add units to each model attribute (see #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
get
function in the same way??
A few unit related problems I noticed:
Ex:
Ex:
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?