Skip to content

Better interface for setParameters #326

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SengerM
Copy link

@SengerM SengerM commented Aug 1, 2025

Purpose

setParameters(Name=value)
setParameters(Name1=value1, Name2=value2)

are more clean and natural than

setParameters("Name=value")
setParameters(["Name1=value1","Name2=value2"])

It should be backwards compatible, meaning that it still accepts the strings.

@CLAassistant
Copy link

CLAassistant commented Aug 1, 2025

CLA assistant check
All committers have signed the CLA.

@syntron
Copy link
Contributor

syntron commented Aug 3, 2025

@SengerM would this work on top of PR #314? it creates the option to use dictionaries for all set*() functions ...

@SengerM
Copy link
Author

SengerM commented Aug 4, 2025

@syntron I think PR #314 is an improvement with respect to the current interface, but the interface from this PR is superior and I would suggest to implement it like this in all the set methods. It is the most natural way.

For quick comparison:

model.setParameters(['Name1=Value1', 'Name2=Value2', ...]) # Current interface (cumbersome, error prone, have to deal with conversion to strings, etc).
model.SetParameters(dict(Name1=Value1, Name2=Value2, ...)) # PR #314 (boilerplate `dict()`, other than that it is acceptable).
model.SetParameters(Name1=Value1, Name2=Value2, ...) # This PR (as good as it gets).

I would also only keep the current interface with strings for backwards compatibility, and add a warning that it will be deprecated in the future.

@syntron
Copy link
Contributor

syntron commented Aug 4, 2025

I see the dict based interface as preferable if lots of options are handled which are defined before as dictionary. My idea is to add an additional function which just translates the argument key based variant to the dict based version.

I would also only keep the current interface with strings for backwards compatibility, and add a warning that it will be deprecated in the future.

This is done in PR #314; I also prepared a commit which would remove the depreciated / old way of setting the options.

@syntron
Copy link
Contributor

syntron commented Aug 4, 2025

Please see commit 6cd65ea of PR #314 ...

@SengerM
Copy link
Author

SengerM commented Aug 5, 2025

I see the dict based interface as preferable if lots of options are handled which are defined before as dictionary.

The "normal Python interface" from this PR can naturally do that (if I understood correctly what you mean):

parameters = {f'Name{i}': i**2 for i in range(999)} # 999 different parameters.
model.setParameters(**parameters)

My advice is to keep it simple and to offer the user an interface he would expect in Python. Less maintenance and more user friendly.

This is also the way it is done in many other packages, e.g. matplotlyb.pyplot.plot, which is defined as matplotlib.pyplot.plot(*args, scalex=True, scaley=True, data=None, **kwargs) instead of matplotlib.pyplot.plot(*args, scalex=True, scaley=True, data=None, kwargs). This allows you to do plot(x, y, color='green', marker='o', linestyle='dashed', linewidth=2, markersize=12) instead of plot(x, y, dict(color='green', marker='o', linestyle='dashed', linewidth=2, markersize=12)), and you can of course do

plot_settings = dict(color='green', marker='o', linestyle='dashed', linewidth=2, markersize=12)
plot(x, y, **plot_settings)

@syntron
Copy link
Contributor

syntron commented Aug 5, 2025

@SengerM I learned something and will check this out - thanks for the explanation!

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