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

Introduce new propagator for nValues constraint based on witness/watch #674

Merged
merged 8 commits into from
Mar 18, 2020

Conversation

ArthurGodet
Copy link
Collaborator

Changes proposed in this PR:

  • Global improvements on the performance of the nValues constraint

@chocoteam/core-developer

@ArthurGodet ArthurGodet requested a review from cprudhom March 16, 2020 17:09
Copy link
Member

@cprudhom cprudhom left a comment

Choose a reason for hiding this comment

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

Can you also update CHANGES.md ?

private IntVar nValue;
private final int n;
private int[] concernedValues;
private IStateInt[] witness;
Copy link
Member

Choose a reason for hiding this comment

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

There is no need for witnesses (or watch literal) to be backtrackable.
Indeed, if a value was considered as watched at a given time, it remains true on backtrack

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, I should use int[] instead of IStateInt[]

super(ArrayUtils.concat(vars, nvalue), PropagatorPriority.LINEAR, true);
this.nValue = nvalue;
n = vars.length;
TIntArrayList list = new TIntArrayList();
Copy link
Member

Choose a reason for hiding this comment

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

Why not using a TIntHashSet instead ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because there is no method sort() on a TIntHashSet, and I think there might be code optimisation to do if the values are sorted.

Copy link
Member

Choose a reason for hiding this comment

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

But why to you the values in concernedValues to be sorted ?
Alternatively, you could sort concernedValues in place using Arrays.sort.

concernedValues = list.toArray();
possibleValues = SetFactory.makeStoredSet(SetType.BITSET, 0, model);
mandatoryValues = SetFactory.makeStoredSet(SetType.BITSET, 0, model);
witness = new IStateInt[concernedValues.length];
Copy link
Member

Choose a reason for hiding this comment

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

Can be initialized with

witness = new int[concernedValues.length];
Arrays.fill(witness, -1);

@codecov-io
Copy link

codecov-io commented Mar 17, 2020

Codecov Report

Merging #674 into master will decrease coverage by 17.6%.
The diff coverage is 78.87%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #674       +/-   ##
============================================
- Coverage     41.51%   23.9%   -17.61%     
+ Complexity     6674    3975     -2699     
============================================
  Files           634     635        +1     
  Lines         40255   40343       +88     
  Branches       7685    7710       +25     
============================================
- Hits          16711    9645     -7066     
- Misses        21875   29623     +7748     
+ Partials       1669    1075      -594
Impacted Files Coverage Δ Complexity Δ
...lver/solver/constraints/IIntConstraintFactory.java 41.21% <ø> (-17.03%) 99 <0> (-58)
...ver/solver/constraints/nary/nvalue/PropNValue.java 73.68% <78.87%> (ø) 24 <24> (?)
...org/chocosolver/solver/search/limits/ACounter.java 0% <0%> (-100%) 0% <0%> (-8%)
...g/chocosolver/solver/constraints/unary/Member.java 0% <0%> (-100%) 0% <0%> (-5%)
.../chocosolver/solver/search/limits/FailCounter.java 0% <0%> (-100%) 0% <0%> (-3%)
...er/solver/search/loop/lns/neighbors/INeighbor.java 0% <0%> (-100%) 0% <0%> (-3%)
...ava/org/chocosolver/examples/set/SetPartition.java 0% <0%> (-100%) 0% <0%> (-4%)
...hocosolver/solver/constraints/unary/NotMember.java 0% <0%> (-100%) 0% <0%> (-5%)
...lver/constraints/extension/binary/BinRelation.java 0% <0%> (-100%) 0% <0%> (-1%)
...lver/constraints/nary/nogood/NogoodConstraint.java 0% <0%> (-100%) 0% <0%> (-3%)
... and 259 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8deb953...f927acd. Read the comment docs.

super(ArrayUtils.concat(vars, nvalue), PropagatorPriority.LINEAR, true);
this.nValue = nvalue;
n = vars.length;
TIntArrayList list = new TIntArrayList();
Copy link
Member

Choose a reason for hiding this comment

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

But why to you the values in concernedValues to be sorted ?
Alternatively, you could sort concernedValues in place using Arrays.sort.

@cprudhom cprudhom merged commit 0fbba1a into master Mar 18, 2020
@ArthurGodet ArthurGodet deleted the nValues branch March 18, 2020 14:14
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.

3 participants