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

Add units to models #180

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add units to models #180

wants to merge 3 commits into from

Conversation

NicolasGensollen
Copy link
Member

First step to #84

  • Define constant.py file with the units
  • Add unit=... to all model attributes

As you can see, some attributes are not that obvious. There are a few cases:

Scalar with dimension:
That's the easy case. For example rated_power ==> unit=KVA

Dimensionless scalar:
Convention I used is to define unit=None. For example name or phase...

List of dimensionless scalars:
Convention I used is to define unit=None

List of scalars with dimension:
Convention I used is to define the unit of the scalars. For example impedance_matrix is a list of Complex, and unit=OHM+"/"+M

List of DiTTo objects:
Convention I used is to define unit=None. For example positions which is a list of position objects. The units of the position attributes are defined in position.py

Attribute which dimension depends on another attribute's value:
That's the hard case. There are very few of those but I couldn't find a clean solution. For example, in capacitor.py, the low and high attribute's dimension depends on the value of mode (can be Volts, Watts, amps... depending on the control mode).

@NicolasGensollen NicolasGensollen added the enhancement New feature or request label Aug 24, 2018
@NicolasGensollen NicolasGensollen self-assigned this Aug 24, 2018
elevation = Float(help="""Decimal elevation (meters)""")
# NOTE: We are not really using lat/long I believe, but mostly coordinates in various format
# TODO: Build the conversion to parse to lat/long from given inputs...
long = Float(help="""Decimal Longitude""", unit=M)
Copy link
Member

Choose a reason for hiding this comment

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

Change long to lng (preferred) or lon. long is a Python builtin.

Copy link
Member

@kdheepak kdheepak Aug 24, 2018

Choose a reason for hiding this comment

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

Alternatively, you can spell it out fully (latitude, longitude etc).

@codecov-io
Copy link

codecov-io commented Aug 24, 2018

Codecov Report

Merging #180 into master will increase coverage by 0.16%.
The diff coverage is 70.62%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #180      +/-   ##
==========================================
+ Coverage   43.96%   44.13%   +0.16%     
==========================================
  Files          74       75       +1     
  Lines       16473    16532      +59     
==========================================
+ Hits         7243     7297      +54     
- Misses       9230     9235       +5
Impacted Files Coverage Δ
ditto/models/timeseries.py 92.85% <ø> (ø) ⬆️
ditto/writers/ephasor/write.py 90.41% <ø> (ø) ⬆️
ditto/writers/opendss/write.py 63.74% <ø> (ø) ⬆️
ditto/readers/dew/read.py 0% <0%> (ø) ⬆️
ditto/modify/system_structure.py 16.89% <0%> (ø) ⬆️
ditto/models/reactor.py 0% <0%> (ø) ⬆️
ditto/readers/synergi/read.py 0% <0%> (ø) ⬆️
ditto/models/meter.py 0% <0%> (ø) ⬆️
ditto/models/weather_layer.py 0% <0%> (ø) ⬆️
ditto/readers/dew/read_new.py 0% <0%> (ø) ⬆️
... and 25 more

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 4b7e894...7c7e60f. Read the comment docs.

Copy link
Contributor

@tarekelgindy tarekelgindy left a comment

Choose a reason for hiding this comment

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

I would strongly reccommend adding units of (decimal/percentage) for the attributes:

  • v_max_pu
  • v_min_pu
  • min_power_factor
  • power_factor
  • per_unit
  • vmin
  • vmax
  • center_tap_perct_1_N
  • center_tap_perct_N_2
  • center_tap_perct_1_2
  • ppercentcurrent
  • qpercentcurrent
  • ppercentpower
  • qpercentpower
  • ppercentimpedance
  • qpercentimpdance
  • cutout_percent
  • cutin_percent
  • rise_limit
  • fall_limit
  • var_injection
  • reserve
  • discharge_rate
  • charging_efficiency
  • discharging_efficiency

This ensures that people don't put 100 when DiTTo is expecting 1.0.

Additionally I think that the attribute:

  • noload_loss
    should not have a unit of None

@NicolasGensollen
Copy link
Member Author

@tarekelgindy, I rebased this PR and solved the conflicts.

@codecov-io
Copy link

codecov-io commented Oct 11, 2018

Codecov Report

Merging #180 into master will increase coverage by 0.16%.
The diff coverage is 70%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #180      +/-   ##
==========================================
+ Coverage   43.98%   44.14%   +0.16%     
==========================================
  Files          75       76       +1     
  Lines       17050    17109      +59     
==========================================
+ Hits         7499     7553      +54     
- Misses       9551     9556       +5
Impacted Files Coverage Δ
ditto/writers/ephasor/write.py 90.12% <ø> (ø) ⬆️
ditto/writers/opendss/write.py 62.5% <ø> (ø) ⬆️
ditto/models/timeseries.py 92.85% <ø> (ø) ⬆️
ditto/models/meter.py 0% <0%> (ø) ⬆️
ditto/models/weather_layer.py 0% <0%> (ø) ⬆️
ditto/modify/system_structure.py 16.7% <0%> (ø) ⬆️
ditto/models/phase_reactor.py 0% <0%> (ø) ⬆️
ditto/readers/dew/read.py 0% <0%> (ø) ⬆️
ditto/readers/synergi/read.py 4.74% <0%> (ø) ⬆️
ditto/models/reactor.py 0% <0%> (ø) ⬆️
... and 25 more

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 d160d4f...e3e9c78. Read the comment docs.

lat = Float(help="""Decimal Latitude""")
elevation = Float(help="""Decimal elevation (meters)""")
# NOTE: We are not really using lat/long I believe, but mostly coordinates in various format
# TODO: Build the conversion to parse to lat/long from given inputs...
Copy link
Collaborator

Choose a reason for hiding this comment

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

Re unit lat/long conversion, I've used pyproj in the past and it worked well. The only downside is that if one forgets to set preserve_units=True during initialization things silently break when the input is not in metres.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants