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

Parameter access should be done using indexing (model["F0"]) rather than getattr (getattr(model, "F0")) #1710

Open
abhisrkckl opened this issue Jan 4, 2024 · 2 comments

Comments

@abhisrkckl
Copy link
Contributor

For obvious aesthetic reasons

@aarchiba
Copy link
Contributor

If you're iterating over parameters, then indeed [p] is preferable to getattr(..., p). But if you are working with a specific one, then (arguably) model.F0 is preferable to model["F0"]. I believe that the original intent was to make referring to specific parameters (the second case) more convenient/natural, with the idea that iterating over them would be a rare and exotic thing to do.

Apart from aesthetic reasons, though, accessing parameters through attributes requires a custom __getattr__ and custom ways to distinguish parameters (that can be in par files) from other attributes of the objects. And it defeats type checking/completion. So in retrospect it is a rather questionable decision. Reversing it would be quite a major API change, though, above and beyond all the work to be done internally.

@abhisrkckl
Copy link
Contributor Author

abhisrkckl commented Feb 15, 2024

My original intention here was to replace getattr(model, p) with model[p] inside loops that iterate over parameters and functions that take a parameter name as an argument.

I am not a fan of parameter access like model.F0, and would prefer model["F0"]. As you said, overriding __getattr__ breaks the IDEs' and linters' ability to infer types, and that sure is inconvenient. However, given the prevalence of this notation inside the codebase, I'm not sure if it is worth the effort to change it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants