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

No/#405 add olm model #505

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
Open

No/#405 add olm model #505

wants to merge 4 commits into from

Conversation

noffermann
Copy link
Collaborator

resolves #405

add OLM type model according to IEEE 738-2012
Delete olmCharacteristic from line entity model
@sonarqubegithubprchecks

This comment has been minimized.

@codecov
Copy link

codecov bot commented Feb 4, 2022

Codecov Report

Base: 78.94% // Head: 78.94% // No change to project coverage 👍

Coverage data is based on head (508fa56) compared to base (eec2bc6).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff            @@
##                dev     #505   +/-   ##
=========================================
  Coverage     78.94%   78.94%           
- Complexity     2131     2133    +2     
=========================================
  Files           265      265           
  Lines          8356     8356           
  Branches        785      785           
=========================================
  Hits           6597     6597           
  Misses         1356     1356           
  Partials        403      403           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@ckittl ckittl left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your draft!

If we can get convergence on this documentation, we can start implementing the model (wouldn't like to merge a documentation, that does not reflect the actual state of code). Currently we are about and on our way to have a release with breaking changes in data scheme. I will have a discussion with the other team mates, if we can't / wouldn't like to include your changes in that release as well.

+--------------+---------+---------------------------------------------+
| id | -- | Human readable identifier |
+--------------+---------+---------------------------------------------+
| T_s | °C | max. line temperature |
Copy link
Member

Choose a reason for hiding this comment

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

Throughout the PowerSystemDataModel our variable naming shifted a bit towards expressiveness. It would be good, if you could consider some changes in the naming. E.g. temperatureMax. Additionally, we have used camel case in the other documentation pages. So, it would be good, if we could stick with the same concept in all places.

Moreover, we have defined some StandardUnits. Maybe you could consider to switch the units to those.

+--------------+---------+---------------------------------------------+
| sq | mm^2 | cross section |
+--------------+---------+---------------------------------------------+
| D_0 | m | Leiterdurchmesser Bewehrung |
Copy link
Member

Choose a reason for hiding this comment

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

As we want to target a wide audience, please use English for the documentation.

+--------------+---------+---------------------------------------------+
| R_dc20 | Ohm/m | Specific DC resistance at 20°C |
+--------------+---------+---------------------------------------------+
| Buendelanzahl| -- | |
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

@@ -59,11 +59,6 @@ Entity Model
| geoPosition | -- | | Line string of geographical locations describing the |
| | | | position of the line |
+-------------------+------+--------------------------------------------------------+
| olmCharacteristic | -- | | Characteristic of possible overhead line monitoring |
Copy link
Member

Choose a reason for hiding this comment

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

Within the line model, we need some reference to the OLM model. That is, that I'd like to keep this field and possibly rename it to olmModel or so. Moreover, it would also be desired, that we can have simple models as well. That said, would you mind proposing a second olm model (in the way with the above file) to keep the functionality of this simple model?

@sonarqubegithubprchecks

This comment has been minimized.

@sonarqubegithubprchecks

This comment has been minimized.

1 similar comment
@sonarqubegithubprchecks

This comment has been minimized.

@sonarqubegithubprchecks

This comment has been minimized.

@sonarqubegithubprchecks

This comment has been minimized.

@sonarqubegithubprchecks

This comment has been minimized.

@sonarqubegithubprchecks

This comment has been minimized.

1 similar comment
@sonarqubegithubprchecks

This comment has been minimized.

@sonarqubegithubprchecks
Copy link

Passed

Analysis Details

0 Issues

  • Bug0 Bugs
  • Vulnerability0 Vulnerabilities
  • Code Smell0 Code Smells

Coverage and Duplications

  • No coverage informationNo coverage information (78.10% Estimated after merge)
  • No duplication informationNo duplication information (0.20% Estimated after merge)

Project ID: edu.ie3:PowerSystemDataModel

View in SonarQube

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

Successfully merging this pull request may close these issues.

Add OLM model
2 participants