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.

Sign up for GitHub

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

Jump to bottom

Named MPS - fix duplicated "ranged" binding constraints #1569

Merged
merged 22 commits into from
Nov 20, 2023
Merged

Named MPS - fix duplicated "ranged" binding constraints #1569

merged 22 commits into from
Nov 20, 2023

Conversation

a-zakir
Copy link
Contributor

@a-zakir a-zakir commented Aug 18, 2023 •
edited
Loading

TODO

  • Fix CI (re-generate reference results)
  • Check impacts on infeasible problem analyzer
  • it broke theses tests (non-exhaustive for the moment):
    • 'named & anonymous mps' tests
    • 'test_constraint' part of 'unit and end-to-end'
    • short-tests /
      053 System Map Editor - 1
      061 Four areas - Grid outages 05
      055 System Map Editor - 3
      060 Four areas - Grid outages 04
    • medium-tests /
      077 KCG on regional dataset 02
      078 KCG on regional dataset 03

see Antares_Simulator_Tests_NR/pull/9
#close #1562

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

57.1% 57.1% Coverage
0.0% 0.0% Duplication

@flomnes flomnes changed the title fix duplicated bindingconstraints Named MPS - fix duplicated "ranged" binding constraints Aug 24, 2023
@flomnes flomnes added the bug Something isn't working label Oct 6, 2023
@a-zakir a-zakir mentioned this pull request Oct 24, 2023
Closed
@pull-request-size pull-request-size bot added size/L and removed size/M labels Oct 30, 2023
@pull-request-size pull-request-size bot added size/M and removed size/L labels Oct 30, 2023
@a-zakir a-zakir mentioned this pull request Oct 31, 2023
Closed
JasonMarechal25
JasonMarechal25 reviewed Nov 3, 2023
View reviewed changes
Copy link
Contributor

@JasonMarechal25 JasonMarechal25 left a comment

Choose a reason for hiding this comment

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

I see you added "_gt" and "_lt" suffixes to constraints name in Loader. Does it create any change in MPS produced?

src/libs/antares/study/binding_constraint/BindingConstraintsRepository.cpp Outdated Show resolved Hide resolved
@a-zakir
Copy link
Contributor Author

a-zakir commented Nov 3, 2023

I see you added "_gt" and "_lt" suffixes to constraints name in Loader. Does it create any change in MPS produced?

absolutely! instead of having 2 constraints with the same name.

@JasonMarechal25
Copy link
Contributor

I see you added "_gt" and "_lt" suffixes to constraints name in Loader. Does it create any change in MPS produced?

absolutely! instead of having 2 constraints with the same name.

Just naming, no difference of results? Any difference in the order of apparition of constraints in the mps files?

@a-zakir
Copy link
Contributor Author

a-zakir commented Nov 3, 2023

I see you added "_gt" and "_lt" suffixes to constraints name in Loader. Does it create any change in MPS produced?

absolutely! instead of having 2 constraints with the same name.

Just naming, no difference of results? Any difference in the order of apparition of constraints in the mps files?

that's what i was saying in the daily for days, in named constraints ('gt' and 'lt'), the order matters!

@a-zakir
Copy link
Contributor Author

a-zakir commented Nov 3, 2023

I see you added "_gt" and "_lt" suffixes to constraints name in Loader. Does it create any change in MPS produced?

absolutely! instead of having 2 constraints with the same name.

Just naming, no difference of results? Any difference in the order of apparition of constraints in the mps files?

that's what i was saying in the daily for days, in named constraints ('gt' and 'lt'), the order matters!

see broken tests list

@a-zakir a-zakir requested a review from flomnes November 10, 2023 09:11
flomnes
flomnes reviewed Nov 10, 2023
View reviewed changes
Copy link
Member

@flomnes flomnes left a comment

Choose a reason for hiding this comment

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

Why is it necessary to take _lt/_gt suffixes into account when comparing binding constraints by name ?

@a-zakir
Copy link
Contributor Author

a-zakir commented Nov 10, 2023

Why is it necessary to take _lt/_gt suffixes into account when comparing binding constraints by name ?

without this dev the comparison ret = ConstraintName("Bc_lt)" < ConstraintName("Bc_gt") = false,
in order to keep the same solution we must add the "_lt" one before the "_gt", thus ret has to be true.

also ConstraintName("BcNOT_FROM_THE-SAME_RANGED_lt)" < ConstraintName("Bc_gt") =false

@flomnes
Copy link
Member

flomnes commented Nov 20, 2023

LGTM @a-zakir Please revert YML files.

@pull-request-size pull-request-size bot added size/S and removed size/M labels Nov 20, 2023
@flomnes flomnes merged commit 9c2546a into develop Nov 20, 2023
2 of 4 checks passed
@flomnes flomnes deleted the fix/bc-duplication branch November 20, 2023 12:50
@sonarqubecloud SonarQubeCloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

52.0% 52.0% Coverage
0.0% 0.0% Duplication

flomnes added a commit that referenced this pull request Nov 21, 2023
@flomnes
Named MPS - fix duplicated "ranged" binding constraints (#1569)
29db6f0 
* fix: bc opBoth -->2 contraints

* update & disable "mps" test

* force run ALL tests

* run ub ci

* split name & ID

* separate Id & series filename

* Revert "separate Id & series filename"

This reverts commit 102583d.

* fix bc comparison

* workflows: pick up develop version

* test updated simtest

* refactor bc comparison

* disable named-mps until ref are regenerated

* back to original

* small changes

* update  function's doc

* plural

* X=X

* tidy

* Revert "disable named-mps until ref are regenerated"

This reverts commit 837e6f5.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JasonMarechal25 JasonMarechal25 JasonMarechal25 left review comments

@flomnes flomnes flomnes approved these changes

Assignees
No one assigned
Labels
bug Something isn't working size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Binding constraints : duplicate lines in named MPS files
3 participants
@a-zakir @JasonMarechal25 @flomnes