Skip to content

feat: enable Basic var and List var to coexist for the local search #1606

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
May 30, 2025

Conversation

zepfred
Copy link
Contributor

@zepfred zepfred commented May 23, 2025

This PR completes the feature and enables the mixed model for local search.

@zepfred
Copy link
Contributor Author

zepfred commented May 23, 2025

@triceo I plan to add documentation about the mixed model once I finish the quick-start change, as I'll have concrete models to use in the explanation.

Copy link
Contributor

@triceo triceo left a comment

Choose a reason for hiding this comment

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

This continues to be suspiciously simple. I am really surprised.

Did you test what happens when strength/difficulty comparisons are used? Did you check entity/value sorter manner? We do not support some of it with list variables, and we need to make sure that this does not change here; and the use cases which work still need to work in the mixed mode.

@zepfred
Copy link
Contributor Author

zepfred commented May 26, 2025

This continues to be suspiciously simple. I am really surprised.

Did you test what happens when strength/difficulty comparisons are used? Did you check entity/value sorter manner? We do not support some of it with list variables, and we need to make sure that this does not change here; and the use cases which work still need to work in the mixed mode.

If there is coverage for the mentioned features, then we know the default behavior remains unchanged. I'll ensure to add tests for the relevant features.

@zepfred zepfred force-pushed the feat/ls-mixed-model branch from 1a78122 to 06da606 Compare May 26, 2025 11:50
@zepfred zepfred force-pushed the feat/ls-mixed-model branch from 165e5c2 to 8223d2c Compare May 26, 2025 17:13
@zepfred zepfred force-pushed the feat/ls-mixed-model branch from 5c951a8 to 185e79c Compare May 27, 2025 18:02
@zepfred zepfred force-pushed the feat/ls-mixed-model branch from e4ba841 to 014d677 Compare May 29, 2025 18:09
Copy link
Contributor

@triceo triceo left a comment

Choose a reason for hiding this comment

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

LGTM after comments resolved.

@zepfred zepfred had a problem deploying to documentation (preview) May 30, 2025 11:27 — with GitHub Actions Failure
@zepfred zepfred force-pushed the feat/ls-mixed-model branch from ef2e929 to 11af3d6 Compare May 30, 2025 11:27
@zepfred zepfred had a problem deploying to documentation (preview) May 30, 2025 11:28 — with GitHub Actions Failure
@zepfred zepfred had a problem deploying to documentation (preview) May 30, 2025 11:37 — with GitHub Actions Failure
Copy link

@triceo triceo merged commit 999d2bf into TimefoldAI:main May 30, 2025
41 of 43 checks passed
@triceo triceo added this to the v1.23.0 milestone May 30, 2025
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