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

use square when possible and stronger filtering #1054

Closed
wants to merge 11 commits into from
Closed

Conversation

jgFages
Copy link
Contributor

@jgFages jgFages commented Jul 21, 2023

practical interest to be confirmed by experiments

@sonatype-lift
Copy link

sonatype-lift bot commented Jul 21, 2023

Sonatype Lift is retiring

Sonatype Lift will be retiring on Sep 12, 2023, with its analysis stopping on Aug 12, 2023. We understand that this news may come as a disappointment, and Sonatype is committed to helping you transition off it seamlessly. If you’d like to retain your data, please export your issues from the web console.
We are extremely grateful and thank you for your support over the years.

📖 Read about the impacts and timeline

@jgFages jgFages requested a review from cprudhom July 21, 2023 15:55
Copy link
Collaborator

@ArthurGodet ArthurGodet left a comment

Choose a reason for hiding this comment

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

Basically, your modifications are correct, but I think we can go way further.
First, there is an error in the isEntailed() method.
Second, I have the impression that the filtering is not always consistent between X and Y and can be improved. For instance, at root node, and if X has an enumerated domain, why not remove all values that are not a perfect square ?
Third, the propagator of such a "simple" constraint should reacts to fine events, don't you think ?

It might look like a "total" revamp of the propagator, but I think it is worth it. What do you think ?

(I can help with the revamp if needed)

@mergify mergify bot dismissed ArthurGodet’s stale review July 25, 2023 07:52

Pull request has been modified.

@ArthurGodet
Copy link
Collaborator

I've simplified the code of updateHoles methods. More especially, I've removed dead code (which gave a welcome speed up, some tests taking only 30% of times compared to what they took before) and standardised the code between similar functions.

@jgFages If you are ok with modifications, I think we can merge the code.

@jgFages
Copy link
Contributor Author

jgFages commented Aug 1, 2023

@ArthurGodet the git history has been corrupted with merge commits (some from 2022), this would make history harder to analyze, so I close this PR and I create a new one here : #1055

I will push a last modification and comment your suggestions there

@jgFages jgFages closed this Aug 1, 2023
@ArthurGodet ArthurGodet deleted the JG_square branch August 2, 2023 09:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants