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

Make sure EMAworkbench outcomes and parameter names are valid Python identifiers #274

Open
EwoutH opened this issue Jun 12, 2023 · 6 comments
Assignees
Labels
Milestone

Comments

@EwoutH
Copy link
Collaborator

EwoutH commented Jun 12, 2023

Make sure EMAworkbench outcomes and parameter names are valid Python identifiers

@EwoutH
Copy link
Collaborator Author

EwoutH commented Jun 18, 2023

@quaquel Since you seem in quite a productive mood, would you like to also resolve this issue before releasing 2.4.1?

@quaquel
Copy link
Owner

quaquel commented Jun 18, 2023

The fix is straightforward. There is a method for it.
It needs to be done in Variable (root of uncertainties, outcomes, constants, etc.), AbstractModel (root for all model classes), and Point (root for Scenario and Policy) I guess.

Main issue is that it will break a lot of existing code (including the final assignment for epa 1361).

Would be better off in a 2.5/3.0 release.

quaquel added a commit that referenced this issue Oct 31, 2023
…dentifier

First step for solving #274. This covers all outcome and parameter classes.

Still remaining are constants and potentially all Point subclasses.
@quaquel quaquel added this to the 2.5.0 milestone Oct 31, 2023
@quaquel
Copy link
Owner

quaquel commented Oct 31, 2023

I started working on this in a branch. Constant, parameters, and outcomes now must be valid python identifiers, and this is explicitly checked. There are 2 open questions:

  1. what type of exception to raise? It seems like a type error is appropriate, but I might be wrong.
  2. Do we also need this for model names, and point names (i.e., scenario, policy, lever)?

@EwoutH
Copy link
Collaborator Author

EwoutH commented Oct 31, 2023

  1. It's not a TypeError I think, since the type can be correct (string) but it's just not a valid identified. I think a ValueError is closest, Maybe something like this?
if not name.isidentifier():
    raise ValueError(f"'{name}' is not a valid Python identifier")
  1. Are these names are used in any context where they might be turned into variable names? If so, I would say yes, otherwise probably no. Except if we want it for consistency / future expandability. If that's the case, we could create in util something like
def valid_python_identifier(name):
    if not name.isidentifier():
        raise ValueError(f"'{name}' is not a valid Python identifier.")

And then just check everywhere:

class Model:
    def __init__(self, model_name, ...):
        valid_python_identifier(model_name)

If we truly want to check it everywhere, and if we can guarantee / standardize that the second variable always is the name, we could even create a decorator.

@quaquel
Copy link
Owner

quaquel commented Nov 1, 2023

  1. I'll go with value error
  2. I don't fully recall where the bug originated from. If I recall correctly, it was related to doing optimizations in which case model and policy would not be required to be valid Python identifiers.

@EwoutH
Copy link
Collaborator Author

EwoutH commented Dec 20, 2023

Deprecated in 2.5, will be removed in 3.0 (see #337).

@EwoutH EwoutH modified the milestones: 2.5.0, 3.0 Dec 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants