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

To/#181 reduced grid model #185

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

To/#181 reduced grid model #185

wants to merge 20 commits into from

Conversation

t-ober
Copy link
Contributor

@t-ober t-ober commented Jan 14, 2022

Resolves #181

@t-ober t-ober added the enhancement New feature or request label Jan 14, 2022
@t-ober t-ober requested a review from a team January 14, 2022 10:38
@t-ober
Copy link
Contributor Author

t-ober commented Jan 17, 2022

!test

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.

The core of the changes you propse look good. However, I'd like to ask you to remove the more "hacky" parts and put a bit more emphasize on the structure.

Comment on lines +170 to +174
ReactivePowerCharacteristic.parse(
s"cosPhiFixed:{(0.0, 0.95)}"
),
1.0.asKiloWatt,
0.95
Copy link
Member

Choose a reason for hiding this comment

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

You may consider adding those parameters to the method signature and assign default values. Once again, you would be more flexible in the way you can apply the method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I only create these system participants so I can overwrite their behavior with primary data so the parameters don't matter and I do not think that this comes in handy anywhere else.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, in that scenario it actually works. However, the model after that is then "implicitly" bound to only be used in conjunction with a primary data source.

I'm a bit worrying about other people coming accross the repository in some months or years and using the created models in a fashion, that is physically / simulation-wise not sensible. Maybay you can add a warning to the README.md then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did add words of warnings to the method descriptions. Since the models don't ship with the code you shouldn't be able to use the models incorrectly and I think with the added comment it should be sufficiently clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did add words of warnings to the method descriptions. Since the models don't ship with the code you shouldn't be able to use the models incorrectly and I think with the added comment it should be sufficiently clear.

@t-ober t-ober requested a review from ckittl February 14, 2022 12:56
@sonarqubegithubprchecks

This comment has been minimized.

@codecov
Copy link

codecov bot commented Mar 23, 2022

Codecov Report

Merging #185 (5b2fd34) into main (57c4f03) will decrease coverage by 2.53%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main     #185      +/-   ##
==========================================
- Coverage   86.45%   83.91%   -2.54%     
==========================================
  Files          40       41       +1     
  Lines         967     1032      +65     
  Branches        2        2              
==========================================
+ Hits          836      866      +30     
- Misses        131      166      +35     
Impacted Files Coverage Δ
...edu/ie3/powerFactory2psdm/main/RunConversion.scala 0.00% <0.00%> (ø)
...la/edu/ie3/powerFactory2psdm/io/PfGridParser.scala
...e3/powerFactory2psdm/reduce/GridModelReducer.scala 50.00% <0.00%> (ø)
...n/scala/edu/ie3/powerFactory2psdm/io/IoUtils.scala 37.83% <0.00%> (ø)

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 57c4f03...5b2fd34. Read the comment docs.

@sonarqubegithubprchecks

This comment has been minimized.

@sonarqubegithubprchecks

This comment has been minimized.

@sonarqubegithubprchecks

This comment has been minimized.

@sonarqubegithubprchecks
Copy link

Passed

Analysis Details

1 Issue

  • Bug0 Bugs
  • Vulnerability0 Vulnerabilities
  • Code Smell1 Code Smell

Coverage and Duplications

  • 40 percent coverage41.67% Coverage (83.90% Estimated after merge)
  • 3 percent duplication0.00% Duplicated Code (0.00% Estimated after merge)

Project ID: edu.ie3:powerFactory2psdm

View in SonarQube

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

Successfully merging this pull request may close these issues.

Reduced grid model for maping of primary data
2 participants