Synergi reader length unit parser

[etimberg]

This was an idea I had based on the discussion in #158 regarding units. I created a separate function that handles unit conversions for Synergi values based on the type (short unit, medium unit, long unit, and per long unit) and the length units from the Synergi DB ('English2', 'English1', and 'Metric').

I added some tests to cover this convert function. For English1, since the units of SUL and MUL are unknown, I assumed they were the same as English2.

I also converted the Synergi reader to use this new convert function which removes some of the hard coded constants that assumed 'English2' units. I would wait to merge this until after the Synergi reader has some tests to be more confident in how it works.

[codecov-io]

Codecov Report

Merging #197 into master will increase coverage by 0.31%.
The diff coverage is 45.94%.

@@            Coverage Diff             @@
##           master     #197      +/-   ##
==========================================
+ Coverage   43.65%   43.97%   +0.31%     
==========================================
  Files          74       75       +1     
  Lines       17027    17038      +11     
==========================================
+ Hits         7433     7492      +59     
+ Misses       9594     9546      -48

Impacted Files	Coverage Δ
ditto/readers/synergi/length_units.py	100% <100%> (ø)
ditto/readers/synergi/read.py	4.88% <4.76%> (+4.88%)	⬆️
ditto/readers/synergi/db_parser.py	26.08% <0%> (+26.08%)	⬆️
ditto/readers/synergi/__init__.py	100% <0%> (+100%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 054c952...a74befc. Read the comment docs.

[NicolasGensollen]

Thanks for starting the effort on this @etimberg! We are definitely planning to work on that soon since the way units are handled right now is really far from optimum...

We were thinking about using a callback when an attribute of the models is set and use Pint to manage the conversion. Basically, when setting an attribute, the user can specify whatever unit he/she wants as long as the unit makes sense for this attribute (providing a length in Ohms for example should raise an Error...).
A conversion function using Pint is then called and will create a Pint object with the value and unit provided, try to convert to the DiTTo units if physically possible, and set the attribute with the new value. This way, the user does not have to care about unit conversion but must still be aware of the units he/she is providing.

Basically, instead of doing something like this in the reader with hardcoded conversion factors:

api_line.length = data_length * 1609.34 # data_length in mi and DiTTo in meters

you would do something like:

api_line.set_attribute("length", data_length, "mi")

set_attribute will then handle conversion and attribute setting. I started setting DiTTo units in the models in PR #180.

Hope this makes sense (more or less). This is something we are still thinking and designing so feel free to give your opinion/ideas!

Concerning this PR, I think it's a good improvement compared to what the current Synergi reader is doing. We probably want to do some testing before merging as you said but this could be a good solution for the Synergi reader before we have a more consistent unit framework in place.

[etimberg]

@NicolasGensollen I like the idea of using another library for the conversion.

I think when creating the wrapper, especially for Synergi, I think it would make sense to abstract away the input unit a bit to avoid if-else checks against the length units in a number of places. I think that would be a simple abstraction though.

if LengthUnits == 'English2':
    api_line.set_attribute("length", data_length, "mi")
elif LengthUnits == 'English1':
    api_line.set_attribute("length", data_length, "kft")
else:
    api_line.set_attribute("length", data_length, "km")

[tarekelgindy]

Thanks a lot for putting this together @etimberg!
I pulled your branch and merged it with the branch synergi_bugfix. Unfortuantely we don't have any great Synergi tests on the repo yet, but I tested it with some Synergi utility feeders we have and it seemed to work well.

The only discrepancy I had with the previous conversion was due to the Per_LUL value for English 2 being 1/1609.34 rather than the decimal value in this pull request, but I'm happy to approve it as is.

[etimberg]

@tarekelgindy thanks for the review. I updated the Per_LuL constant for English2 to be 1/1609.34