Repository navigation
API 2.0: Improve parameters in existing code #1049
Description
As we work toward OPS 2.0, we have the opportunity to improve the OPS user experience by changing confusing parameters/defaults in the existing code. This issue is a place to collect examples of parameters that may cause use confusion, and to discuss improvements. For anything that might be a long discussion, we might split off a separate issue.
This issue is a suitable place to suggest situations where we should:
- Remove/change defaults.
- Reorder parameters.
- Group/ungroup parameters into a container for multiple parameters.
- Rename parameters with unclear names (see also #986)
As we find more of these, I'll add them to the task list in this post. Try to propose one suggestion per comment; if you agree but have nothing to add to a specific suggestion, please react to that comment with a 👍. If you disagree, feel free to give it a 👎, but also please provide feedback.
- Make
enginea required parameter in strategies that use engines (discussion in openpathsampling/ops_tutorial#17).
Related issues:
Activity
[-]API 2.0: Improve parameters to existing code[/-][+]API 2.0: Improve parameters in existing code[/+]dwhswenson commentedon Aug 16, 2021
Convert engine
optionsto keyword-only argumentsCurrently, engines take an
optionsdictionary. This is a horrible design decision for many reasons. It's hard to document the allowed options. Typos in option names have to be checked (or you get unexpected default values). It's a somewhat unnatural way for users to set these options.I suggest switching these to parameters. In most cases, that will be keyword-only parameters (for some, like the
integratorforToyEngine, it will be positional-or-keyword).So instead of:
you have
Some tools to measure code quality *ahem CodeClimate ahem* may complain about too many parameters, but the reason to avoid lots of parameters is that a user won't remember which is which -- keyword-only already solves that problem. Additionally, since we can have required (no default) keyword-only parameters, I think that many engine options should be required keyword-only (what is a reasonable default for
n_steps_per_frame? We currently use10in the OpenMM engine, which is reasonable for alanine dipeptide and not much else).(Note: this is an example of what I meant in the top post by "group/ungroup parameters into a container" -- this is ungrouping them.)
dwhswenson commentedon Aug 16, 2021
Use keyword-only arguments for optional parameters in strategies
I might be going too far in the direction of keyword-only on this (would appreciate feedback!) but I think that optional parameters (including, e.g.,
selectorfor shooting strategies) should be keyword-only. Due to the nature of these objects as factories, binding the wrong object to a given parameter name only shows up much later at runtime. One solution for that would be to do internal type checking. I think the more Pythonic approach would be to make these options keyword-only.Again, I tend to be very heavy in my use of keyword parameters when coding, so I'd like feedback from other users who may have other coding styles to see if requiring keywords would be annoying.
sroet commentedon Aug 18, 2021
Just my feedback: I am also keyword based, so not much pushback from me on that. I am +1 on any solution that makes this error out on initialization instead at runtime
hejung commentedon Aug 23, 2021
I am also very much in favor of keyword only arguments instead of the dictionaries (tab completion, yay!).
One thing to consider when doing this is that (at least for me) having default values results in me thinking less about the value of a parameter, so it might even be good to force every user to explicitly set (and think about) things like
n_steps_per_framefor which we do not have universal defaults because they are very system dependent. So I think it makes sense to think hard if we know a sensible default value (i.e. if the respective parameter really is optional).