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

[SLD] add limit violation highlight #643

Merged
merged 30 commits into from
Oct 15, 2024
Merged

Conversation

jamal-khey
Copy link
Contributor

@jamal-khey jamal-khey commented Sep 3, 2024

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce?
feature:
This PR add a CSS class to Lines/Transformer/Bus in case of a limit violation

expected result, similar to what is done for NAD, we want to be able to highlight the equipment that has a limit violation
example 1
example 2

similar animation is also applied to busbarsection :

example 3

What is the current behavior?
the label is added but without informations if the equipment is in limit violation

What is the new behavior (if this is a feature change)?
add a CSS class sld-overload sld-vl-overvoltage sld-vl-undervoltage

Does this PR introduce a breaking change or deprecate an API?

  • Yes
  • No

Modifications in animations.css

@jamal-khey jamal-khey self-assigned this Sep 3, 2024
@jamal-khey jamal-khey requested a review from flo-dup September 3, 2024 16:21
Signed-off-by: jamal-khey <[email protected]>
@jamal-khey jamal-khey removed the request for review from flo-dup September 3, 2024 17:10
@jamal-khey jamal-khey changed the title [SLD] add sld-overload class to equipment label [WIP] [SLD] add sld-overload class to equipment label Sep 3, 2024
@jamal-khey jamal-khey changed the title [WIP] [SLD] add sld-overload class to equipment label [SLD] add sld-overload class to equipment label Sep 5, 2024
@jamal-khey jamal-khey requested a review from flo-dup September 5, 2024 12:07
Signed-off-by: jamal-khey <[email protected]>
Copy link
Member

@So-Fras So-Fras left a comment

Choose a reason for hiding this comment

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

Thank you for this PR. I have a few points that are unclear to me:

  • Shouldn't we add the style in the .css?
  • What kind of equipment could be overloaded? Here I am under the impression that a connectivityNode could be "overloaded". Does it make sense?
  • I am not sure that we should modify the AbstractStyleProvider. We could do the same as what we did with the HighlightLineStateStyleProvider

To be further discussed!

@flo-dup
Copy link
Contributor

flo-dup commented Sep 12, 2024

Shouldn't we add the style in the .css?

Indeed, could you add the css from gridsuite/gridstudy-app#2244?

What kind of equipment could be overloaded? Here I am under the impression that a connectivityNode could be "overloaded". Does it make sense?

Indeed, I think overloaded is now everywhere in the graph, but we tried to avoid to populate the graph with data which is not needed for the diagram structure. The way we handle this is to look in the network when requesting the classes on a particular node or a particular edge, like in HighlightLineStateStyleProvider

I am not sure that we should modify the AbstractStyleProvider. We could do the same as what we did with the HighlightLineStateStyleProvider

Indeed, we'd rather do it like that

@flo-dup
Copy link
Contributor

flo-dup commented Sep 12, 2024

About the rendering, if we want to keep the same rendering as the connect / disconnect styles from HighlightLineStateStyleProvider, we should have only the last edge blinking, not the whole cell like in the unit tests. And if it's a transformer which is overloaded, shouldn't it be only the transformer which is blinking?

@jamal-khey jamal-khey requested a review from So-Fras September 16, 2024 13:03
@jamal-khey jamal-khey force-pushed the jamalkhey/add-overloadStyle branch from 9a0530a to 9768bfd Compare September 18, 2024 13:33
jamal-khey and others added 3 commits September 24, 2024 11:45
undo previous changes, add new LimitHighlightStyleProvider

Signed-off-by: jamal-khey <[email protected]>
Signed-off-by: jamal-khey <[email protected]>
add unit test for under/over voltage limit

Signed-off-by: jamal-khey <[email protected]>
Signed-off-by: jamal-khey <[email protected]>
@jamal-khey jamal-khey force-pushed the jamalkhey/add-overloadStyle branch from b414d57 to 0c294d0 Compare September 24, 2024 09:45
@jamal-khey
Copy link
Contributor Author

jamal-khey commented Sep 24, 2024

Indeed, I think overloaded is now everywhere in the graph, but we tried to avoid to populate the graph with data which is not needed for the diagram structure. The way we handle this is to look in the network when requesting the classes on a particular node or a particular edge, like in HighlightLineStateStyleProvider

I reread the PR and realized that we did not fully address @flo-dup 's comment. What he means is that, if possible, we do not want to add the "isOverloaded" (or "isLimitExceeded") kind of information to the graph because it is not necessary to build it. The graph is about Node and Edge and we try not to include "power network" kind of information in it.

Yes you are right @So-Fras , i miss understood the comment.

When we had to do something similar to your PR in the past, we proceeded as in the HighlightLineStateStyleProvider. We linked the Node or Edge to equipment of the network and then we assessed the "state" of the Node or Edge. That is why the HighlightLineStateLineStyleProvider's constructor takes a Network as a parameter.

Could you try to do the same here? Else in the HighlightLineStateStyleProvider (but the name of the class should be changed) or in another specific StyleProvider (maybe better).

I have made the necessary changes to align with this approach, similar to the HighlightLineStateStyleProvider , i added LimitHighlightStyleProvider

@jamal-khey jamal-khey changed the title [SLD] add sld-overload class to equipment label [SLD] add limit violation highlight Sep 24, 2024
Signed-off-by: jamal-khey <[email protected]>
Copy link

@jamal-khey jamal-khey requested a review from flo-dup October 10, 2024 15:42
@So-Fras
Copy link
Member

So-Fras commented Oct 14, 2024

This PR fixes the #590 issue.

@flo-dup flo-dup linked an issue Oct 15, 2024 that may be closed by this pull request
@So-Fras So-Fras merged commit 1f39c39 into main Oct 15, 2024
7 checks passed
@So-Fras So-Fras deleted the jamalkhey/add-overloadStyle branch October 15, 2024 10:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

StyleProvider to highlight overloaded lines in SLD
4 participants