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

UUID #402

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

UUID #402

wants to merge 6 commits into from

Conversation

Wout4
Copy link
Collaborator

@Wout4 Wout4 commented Aug 21, 2023

Added an ID fields to variables, using uuid to get unique id's.
Naming stays the same, for display/print reasons.
When posting to the solver I decided to use name + id because

  • it will still be unique
  • minizinc names have to start with a letter, this ensures that and makes it consistent for all solvers
  • might make debugging easier as the var name is still part of the solver var name

Let me know if you think this is silly.

@363734
Copy link
Contributor

363734 commented Aug 22, 2023

I think this will also be useful for the CSE, as you can then create easily a unique hash for each expression based on the type of operation and the argument of it

@Wout4
Copy link
Collaborator Author

Wout4 commented Aug 22, 2023

defining a unique hash for every expression is a great idea, should probably go in another pr though

@Wout4 Wout4 requested a review from tias August 22, 2023 14:42
return hash(self.name)
# for backwards compatability
if not hasattr(self, 'id'):
self.id = self.id = uuid.uuid4()
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo?

@JoD
Copy link
Collaborator

JoD commented Sep 11, 2023

This is hard to review because of the autoformatting changes. Would it be possible to add these in a separate MR? Also, provide an explanation for all collaborators on how to use the same autoformatter, so that the autoformatting does not get broken in future MRs.

@JoD
Copy link
Collaborator

JoD commented Sep 11, 2023

Replace str(var) + str(var.id) and var.name + str(var.id) with helper function var.solver_name().

@JoD
Copy link
Collaborator

JoD commented Sep 11, 2023

I'm lukewarm on using a uuid. The alternative is using a global variable counter. The advantage of a global counter is that it will also be faster: long string names of variables do have a measurable impact (both for memory and efficiency) in scenario's with lots of variables passed to a solver, so keeping them short (via a counter starting from 0) is better.

However, this is just an optimization issue, so if you think the current approach is fine, that's fine by me.

@363734
Copy link
Contributor

363734 commented Sep 11, 2023

global variable counter does not work with pickled models, as the counter is not pickled. For example, you define a first model with variable id 1 to 10, counter is at 10, you pickle the model, fine. Then you unpickle it in another run, counter is resetted, you decide to add new var to model, var id 1 reused, conflict

@JoD
Copy link
Collaborator

JoD commented Sep 12, 2023

Ah, did not think of that. The counter is indeed part of the state and should be pickled too, but I can see that this may tip the scales in the balance between simplicity and efficiency.

@Wout4
Copy link
Collaborator Author

Wout4 commented Sep 19, 2023

I will try to make a case again for why we need unique identifiers :) (to implement CSE)

After looking into python dicts/sets it turns out that in the event of a hash collision __eq__ gets called on the keys, to decide if they are the same (so not the pointer, nor is)
Since we overwrite eq for cpmpy expressions to return a new comparison object, eq always returns a 'truthy' value.
So 2 expressions with the same hash can never co-exist in a dictionary where we use the expressions themselves as keys.
This means we will need unique hashes if we want to use expressions as keys. (-> uuid needed)

On the other hand, we could use something else as the key, to avoid the problem with overwriting eq. But then we would still need a unique property of expressions to use as the key, since the string representation could not be unique (when the user gives particular names or descriptions to the variables/expressions) (-> uuid needed)

@Wout4 Wout4 linked an issue Jan 8, 2024 that may be closed by this pull request
@Dimosts Dimosts marked this pull request as draft May 2, 2024 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants