Skip to content

Conversation

IgnaceBleukx
Copy link
Collaborator

Allows a user to post a set instead of list of assumption variables during the solve call.
More of a user-experience improvement (personally also encountered this multiple times already) and fixes #689

Could be part of a larger discussion on what to do with sets in general, do we want to support them as if they were lists?
E.g., should get_variables(some_set_of_constraints) also work?

@IgnaceBleukx IgnaceBleukx added the simple to review Simple change to review, e.g., a oneliner. label Aug 4, 2025
@IgnaceBleukx IgnaceBleukx requested a review from ThomSerg August 5, 2025 10:16
Copy link
Collaborator

@ThomSerg ThomSerg left a comment

Choose a reason for hiding this comment

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

Looks good. But was wondering why we don't check if its an iterable instead? Something similar as is done in _flatten(). Not that this would make any noticeable difference. Just a thought, pull request can be merged as is.

@IgnaceBleukx
Copy link
Collaborator Author

IgnaceBleukx commented Aug 6, 2025

Hmm good point, from a user-perspective it might actually be nice if we just took care of any collection.
I'll adapt it so it calls flatlist(assumptions) instead.

@IgnaceBleukx IgnaceBleukx requested a review from ThomSerg August 7, 2025 10:50
@tias
Copy link
Collaborator

tias commented Aug 18, 2025

I'm not convinced... we should have stricter typing, not weaker?

e.g. this should be a List[BoolVarImpl_] or smth...

if a user has a set, they should just do list({a,b})? Or it can be any Iterable, but 'flatten' means also nested lists?

@IgnaceBleukx
Copy link
Collaborator Author

Hmm yeah explicitly typing this might indeed be the better option, we should check for it as well then and raise a proper error

@hbierlee
Copy link
Contributor

hbierlee commented Aug 26, 2025

I'd be in favor of an Iterable over literals (without flattening).

On one hand, this is weaker typing than having List[..] which is good: it only requires what is needed (least knowledge principle, I believe). In this case, we only require something we can iterate over to get each next assumption, and whether the assumptions come from a list, set, or some expensive asynchronous generator, doesn't matter to us. In this case, the user may have a good reason to keep assumptions in a set, and it's unnecessary/inconvenient/maybe costly to convert this set to a list.

On the other hand, this typing is stronger b/c it does not accept iterators over non-literals: e.g. the iterable should be "flat" (only generate literals, not suddenly generate lists!), because it's not clear what this means. An assumption is a literal, not a list.

@hbierlee
Copy link
Contributor

Also note: in this PR, please also remove ocus_explanations from skipped examples in test_examples and remove TODO comment in the example code in this PR too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
simple to review Simple change to review, e.g., a oneliner.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[OR-Tools] Ocus assumptions: TypeError: unhashable type: 'set'
4 participants