Skip to content

chore: move streams change moves for list variable #1727

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

Merged
merged 18 commits into from
Aug 12, 2025

Conversation

triceo
Copy link
Collaborator

@triceo triceo commented Aug 7, 2025

Introduces ListChangeMoveProvider that is capable of generating change moves, assign moves and unassign moves.

To do that, several new capabilities are introduced to data streams, which mimic capabilities of constraint streams and use the same underlying Bavet nodes.

With the new value range work, nodes and data sessions no longer need initialization, because everything we need, we can now find in the value range manager.

@triceo triceo added this to the v1.25.0 milestone Aug 7, 2025
@triceo triceo requested a review from zepfred August 7, 2025 06:51
@triceo triceo self-assigned this Aug 7, 2025
@Copilot Copilot AI review requested due to automatic review settings August 7, 2025 06:51
Copilot

This comment was marked as outdated.

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request introduces ListChangeMoveProvider that generates change moves, assign moves, and unassign moves for list variables. The implementation adds new capabilities to data streams and removes initialization requirements from nodes and data sessions by utilizing the value range manager.

  • Adds comprehensive list variable move generation with support for assignments, changes, and unassignments
  • Introduces mapping and distinct operations to data streams with new APIs
  • Removes node initialization requirements and simplifies session management

Reviewed Changes

Copilot reviewed 80 out of 80 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
ListChangeMoveProvider.java New provider generating list variable moves with filtering logic
DefaultSolverTest.java Adds test for list variable solving with move streams
SolutionView.java Adds countValues method for list variable length queries
AbstractUniDataStream.java Implements map and distinct operations for data stream transformations
DefaultMoveStreamFactory.java Updates constructor to accept EnvironmentMode parameter
Various move classes Updates move implementations with better type safety and getter methods
Comments suppressed due to low confidence (1)

core/src/test/java/ai/timefold/solver/core/impl/solver/DefaultSolverTest.java:444

  • Grammatical error: 'Only penalizes is length' should be 'Only penalizes if length'.
            }

@triceo
Copy link
Collaborator Author

triceo commented Aug 8, 2025

The concern regarding distinct() is not well founded, for three reasons:

  • This is an operation in the underlying Bavet network, and is fully incremental. (Under the hood, it is implemented using a GroupBy node.) It does have some overhead, but any Bavet node has overhead. In this case, the overhead is comparable to map, which does a very similar thing.

  • CS uses the same underlying Bavet network, and its network is updated on every move. MS, on the other hand, only updates its Bavet network once at the end of every step. Therefore, if we assume that the performance of CS is sufficient, the performance of MS will be pretty much irrelevant, because it will be executed much less than CS.

  • This is an implementation detail. If it ever proves insufficient, we will change the implementation of the provider. At the moment, I am worried about functionality and the programming model that we expose, not minor performance issues.

@zepfred
Copy link
Contributor

zepfred commented Aug 11, 2025

The concern regarding distinct() is not well founded, for three reasons:

  • This is an operation in the underlying Bavet network, and is fully incremental. (Under the hood, it is implemented using a GroupBy node.) It does have some overhead, but any Bavet node has overhead. In this case, the overhead is comparable to map, which does a very similar thing.
  • CS uses the same underlying Bavet network, and its network is updated on every move. MS, on the other hand, only updates its Bavet network once at the end of every step. Therefore, if we assume that the performance of CS is sufficient, the performance of MS will be pretty much irrelevant, because it will be executed much less than CS.
  • This is an implementation detail. If it ever proves insufficient, we will change the implementation of the provider. At the moment, I am worried about functionality and the programming model that we expose, not minor performance issues.

Thank you for the explanation. Having the GroupBy at the end of the step is indeed better than having it at the end of each applied move. However, if it is not necessary, avoiding it altogether is even better.

Nevertheless, I liked the functionality and programming model provided at this stage.

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
54.2% Coverage on New Code (required ≥ 70%)

See analysis details on SonarQube Cloud

@triceo triceo merged commit 16e2464 into TimefoldAI:main Aug 12, 2025
33 of 34 checks passed
@triceo triceo deleted the listvar branch August 12, 2025 06:24
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