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

Adjusted the Export script #29

Merged
merged 17 commits into from
May 18, 2021
Merged

Adjusted the Export script #29

merged 17 commits into from
May 18, 2021

Conversation

t-ober
Copy link
Contributor

@t-ober t-ober commented Apr 22, 2021

Resolves #17

  1. adjusted the export script to include connected elements for generating the grid topology.
  2. changed the logic to mention fields to include rather than fields to exclude to make developing easier and add necessary parameters step by step.

@t-ober t-ober added the enhancement New feature or request label Apr 22, 2021
@t-ober t-ober requested review from johanneshiry and ckittl April 22, 2021 08:42
@codecov
Copy link

codecov bot commented Apr 22, 2021

Codecov Report

Merging #29 (e20b797) into main (384605a) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main      #29   +/-   ##
=======================================
  Coverage   77.47%   77.47%           
=======================================
  Files           4        4           
  Lines         111      111           
  Branches        1        1           
=======================================
  Hits           86       86           
  Misses         25       25           

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 384605a...e20b797. Read the comment docs.

@johanneshiry
Copy link
Member

Is this still up 4 review? I thought we discussed that it might not make sense to have it that way? 🤔
If it's not up 4 review pls consider either close it or convert it to a draft.

@t-ober
Copy link
Contributor Author

t-ober commented May 2, 2021

Is this still up 4 review? I thought we discussed that it might not make sense to have it that way? 🤔
If it's not up 4 review pls consider either close it or convert it to a draft.

@johanneshiry
Do you refer to the export connected elements part (1) or the include rather than exclude fields (2) ?

Regarding 1: We still need the connected elements for building the grid topology to map the different components to the nodes they are connected to.

Regarding 2: I thought we came to the conclusion that it would make sense to go for that approach while developing, as it makes the process of incrementally including new parameters much more seamless.

So I think that it still makes sense, but I am happy to discuss it.

@ckittl
Copy link
Member

ckittl commented May 12, 2021

!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.

I really like, that you do not overwhelm the user with documentation. ;-)

src/main/python/powerFactory2json/pf2json.py Outdated Show resolved Hide resolved
src/main/python/powerFactory2json/pf2json.py Outdated Show resolved Hide resolved
src/main/python/powerFactory2json/pf2json.py Outdated Show resolved Hide resolved
src/main/python/powerFactory2json/pf2json.py Outdated Show resolved Hide resolved
src/main/python/powerFactory2json/pf2json.py Outdated Show resolved Hide resolved
src/main/python/powerFactory2json/pf2jsonUtils.py Outdated Show resolved Hide resolved
@t-ober
Copy link
Contributor Author

t-ober commented May 13, 2021

I really like, that you do not overwhelm the user with documentation. ;-)

THANKS! Finally someone who noticed 😂

@t-ober
Copy link
Contributor Author

t-ober commented May 13, 2021

!test

1 similar comment
@t-ober
Copy link
Contributor Author

t-ober commented May 14, 2021

!test

@t-ober t-ober requested a review from ckittl May 14, 2021 08:12
@ckittl ckittl merged commit 572763b into main May 18, 2021
@ckittl ckittl deleted the to/#17-export-conElms branch May 18, 2021 09:44
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.

Adapt export script to include connected elements
3 participants