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

[RomApp] Fix Bug in ECM #11732

Merged
merged 11 commits into from
Nov 15, 2023
Merged

[RomApp] Fix Bug in ECM #11732

merged 11 commits into from
Nov 15, 2023

Conversation

Rbravo555
Copy link
Member

@Rbravo555 Rbravo555 commented Oct 27, 2023

📝 Description
This PR is intended to fix two problems in the ECM algorithm:

  1. The extra constraint that until now was always enforced that the sum of the HROM weights was equal to the number of elements, sometimes ill-conditions the problem. We are now adding a flag, for the user to set this constraint to false if required. The default is still for it to be true, although we can discuss it further
  2. There was a recently introduced bug consisting in, whenever the number of iterations was larger that some hard-coded large value (1000), the ECM tried to "enlarge" the candidate set. We intended this for the case of weakly enforcing the ECM to contain some initial candidate set, and then enlarge if convergence was impossible. For ill-conditioned problems like the ones arising from the prior point, many iterations occurred and when the ECM tried to enlarge the candidates, the so-called complement_set did not exist. The fix is as simple as enlarging only if the complement exists.

🆕 Changelog

  • In ECM enlarge candidates with complement only if complement exists
  • Add flag to control whether the "sum_of_elements" constraint should be enforce for the ECM. This affects:
    • RomManager
    • HromTrainingUtility

@Rbravo555 Rbravo555 requested a review from a team as a code owner October 27, 2023 16:26
@Rbravo555 Rbravo555 marked this pull request as draft October 27, 2023 16:28
@Rbravo555 Rbravo555 changed the title Rom app fix bug ecm [RomApp] Fix Bug in ECM Oct 27, 2023
@RiccardoRossi
Copy link
Member

just a question: not enforcing that constraint implies giving back with the capability of exactly integrating the areas, is that correct?

@Rbravo555
Copy link
Member Author

just a question: not enforcing that constraint implies giving back with the capability of exactly integrating the areas, is that correct?

The intention of the constraint is to avoid the trivial solution of all weights equal to zero. A by-product of the contraint is that the sum of the weights equal the number of elements. If the contraint is not enforced, the sum of the weights can be whatever value the greedy algorithm comes up with, which can be very large or very small according to the specific problem. Imposing this condition should not have a negative effect besides requiring a single element extra, with respect to not imposing it. However, in practice we are observing that enforcing this constraint sometimes prevents the algorithm from converging. We are looking into the reasons why this is happening. In the meantime, this PR provides an easy fix by allowing the user to choose not to impose the constraint if convergence is difficult for the specific case.

@Rbravo555 Rbravo555 self-assigned this Oct 30, 2023
@Rbravo555 Rbravo555 marked this pull request as ready for review October 30, 2023 10:40
@SADPR
Copy link
Contributor

SADPR commented Oct 30, 2023

Can you please take a look why this is not passing the CI?
CI / ubuntu-intel-legacy (pull_request)

@@ -17,7 +17,7 @@ class EmpiricalCubatureMethod():
def __init__(
self,
ECM_tolerance = 0,
Filter_tolerance = 1e-16,
Filter_tolerance = 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the tolerance here set to 0 by default?
I mean, do you think it will most likely need to use all elements/conditions to converge?

@SADPR
Copy link
Contributor

SADPR commented Oct 30, 2023

Everything else LGTM.

@Rbravo555
Copy link
Member Author

Could we approve this then?

@RiccardoRossi
Copy link
Member

no objection on my side.

Copy link
Contributor

@SADPR SADPR left a comment

Choose a reason for hiding this comment

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

LGTM

@SADPR SADPR merged commit ce8f35c into master Nov 15, 2023
10 of 11 checks passed
@SADPR SADPR deleted the RomApp_FixBugECM branch November 15, 2023 09: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.

None yet

3 participants