search

BreakingBytes / simkit

Model Simulation Framework
http://breakingbytes.github.io/simkit/
BSD 3-Clause "New" or "Revised" License
27 stars 16 forks source link

inconsistent use of None with units #103

Open mikofski opened 7 years ago

mikofski commented 7 years ago

Not sure, but the use of None with units especially when calling __getitem__() with no default on parameters, since the default if not given is None. Pint will create a dimensionless unit if None or "" (or nearly anything that evaluates to False) are given.

@anomam I think we need to careful of this and be as explicit as possible in unit declarations, use "dimensionless" or "" for non-dimen units, otherwise, leave the units arg out of the parameter call.

anomam commented 7 years ago

Thanks for raising that issue @mikofski . I'm actually not too sure about how to deal with the following: Let's say that I have a formula f_soiling(soiling_mode, rainfall=None) where soiling_mode has no units and rainfall is in mm. It looks like I always need to specify all of the inputs in the calculations, even if I don't need rainfall in the case of static soiling. But if I remember correctly, Carousel will yell at me if I try to pass rainfall = None * UREG['mm'] because as you mentioned, the unit is changed to dimensionless or "". What do you think I should do here?

mikofski commented 7 years ago

IMO I think that is a different issue. I'm referring to how units are sometimes set to None or how inferences are made when the value of either data or formula attributes are set to None. For example in on line 175 in the data reader, if the units attribute is missing, I think the units are set to None by default, which would be inferred as dimensionless. I'm not sure if this is even a problem, but it seems inconsistent, since sometimes we want this to mean that there are no units at all.

For the situation you described, I think you could change the default value of rainfall to zero or -1, in case you need it to be a flag.

f_soiling(soiling_mode, rainfall=-1):
    if rainfall < 1:
        <do stuff you would have done if rainfall had been None>
    else:
        <do stuff you would have done if rainfall had not been None>

Then if the units of rainfall are mm, this will work fine even for the cases when rainfall is not used.

Yes, you are correct, to use the Pint units wrapper, you must specify units for all arguments, unless the strict=False flag is set, which in Carousel is set to strict=True by default, sorry. :frowning_face: