Skip to content

Non-strict var name check#839

Open
ThomSerg wants to merge 5 commits intomasterfrom
var_name_check
Open

Non-strict var name check#839
ThomSerg wants to merge 5 commits intomasterfrom
var_name_check

Conversation

@ThomSerg
Copy link
Copy Markdown
Collaborator

Added some internal tools to have greater control over variable name checks.
When working on file readers and writers, I encountered the issue that we couldn't read the model files that we ourselves have exported, due to the use of IV and BV decision variable namings. In this PR I added a couple of helper functions to allow for these variable names as long as they haven't been used before.

Now, file readers can wrap their logic in

with ignore_strict_variable_name_check():
    ... create CPMpy model based on file contents here ...

This will temporarily allow the use of restricted variable names (but still checks for variable name collisions).

Additionally I also extracted the "update variable counters from model" logic used when loading models from .pickle files to make it reusable (_update_variable_counters) (file readers will also need this)

For now, readers will still have to update the counters themselves after exiting ignore_strict_variable_name_check, i.e. after creating the CPMpy model they should call:

_update_variable_counters(model)

But I might have found a way to do this automatically (that could potentially also resolve the issues when copying CPMpy models across processes, but to be seen / no promises) (different approach than Ignace's). Will be for a follow-up PR.

Copy link
Copy Markdown
Collaborator

@IgnaceBleukx IgnaceBleukx left a comment

Choose a reason for hiding this comment

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

Very elegant use of the context manager : )
I would just make the function call also private, to really indicate this is non-trivial usage of the library.

_disable_strict_variable_name_check()
def __exit__(self, exc_type, exc_value, traceback):
_enable_strict_variable_name_check()
# _update_variable_counters() # TODO: add automatic support for this later (different PR)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

So for that you would need access to the model here?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yes, with the current update function you would indeed. So currently the only approach to do it somewhat automatic is to create you model before entering the context, and then call:

with ignore_strict_variable_name_check(model=model):
    ...

The context manager then has the model and can call the update for you on exit.

Copy link
Copy Markdown
Collaborator

@tias tias left a comment

Choose a reason for hiding this comment

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

fine by me...

will also be of benefit in other file readers I understand?

If Ignace has no further comments then can go in...

maybe do add your example from the comment in the Docstring

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