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

166 single group element #208

Merged
merged 20 commits into from
Nov 1, 2023
Merged

166 single group element #208

merged 20 commits into from
Nov 1, 2023

Conversation

ShiriMoran
Copy link
Contributor

No description provided.

pkg/vpcmodel/grouping.go Outdated Show resolved Hide resolved
Comment on lines 308 to 309
res, groupingSrcOrDst := g.groupLinesByKey(srcGrouping, groupVsi)
res = g.unifiedGroupedConnLines(res)
Copy link
Collaborator

Choose a reason for hiding this comment

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

How is res affected by extendGroupingSelfLoops? It seems that only groupLinesByKey adds lines to res, so why on this stage the func unifiedGroupedConnLines works on res?

Copy link
Contributor Author

@ShiriMoran ShiriMoran Nov 1, 2023

Choose a reason for hiding this comment

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

@adisos good catch. So there are 2 questions to answer here: yours, and why still the fix that seems misplaced solved the issue, as per my testing and Haim's.
The original problem here is that extendGroupingSelfLoops "mess up with" the basic result of the grouping, modifying both sides and not only the targeted one. This is why we had a problem in the first place.
The fix I've put in indeed does not cover the problem, as you say. The reason it did seem to cover the problem is that is fixed the result res of the previous stage and for the case inline it was suffice. Clearly this is not a proper solution. I'll fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed @haim-kermany please verify it is working as expected (by my testing it is)

@haim-kermany
Copy link
Contributor

the code works as eccpected, tnx

@ShiriMoran ShiriMoran merged commit e4eb12f into main Nov 1, 2023
4 checks passed
@ShiriMoran ShiriMoran deleted the 166_single_groupElement branch November 1, 2023 11:59
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.

please do not create two instancesof groupedExternalNodes of the same set of nodes
3 participants