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

Settings object should become immutable once the model is setup #1070

Open
cprudhom opened this issue Nov 8, 2023 · 2 comments
Open

Settings object should become immutable once the model is setup #1070

cprudhom opened this issue Nov 8, 2023 · 2 comments
Labels

Comments

@cprudhom
Copy link
Member

cprudhom commented Nov 8, 2023

This way of declaring settings is correct:

Model model = new Model(Settings.init().setWarnUser(false));

because the Settings instance is not stored and cannot be modified once the model is created.
But the following declaration makes possible to change the settings afterward:

Settings settings = Settings.init().setWarnUser(false);
Model model = new Model(settings);
settings.setWarnUser(true);

I suppose the settings must be set immutable or the model should have a deep copy of it.

@cprudhom cprudhom added the bug label Nov 8, 2023
@jgFages
Copy link
Contributor

jgFages commented Nov 15, 2023

I see your point, but I would not tag it as a bug.

It is true that some settings cannot be modified afterwards (I think only the setInitSolver is concerned, because the initSolver is called in the constructor). Nevertheless, for the vast majority of the settings, we can update their value without problem, and I find the following code more convenient than going through the model constructor :
Model model = new Model(); model.getSettings().setEnableSAT(true);
So, even if I agree that we could have something cleaner/safer, it may make the models heavier... so I may prefer to keep it as it is (not a strong opinion either).

Did you want to somehow "factorise" the same settings object for several Models ?

@ArthurGodet
Copy link
Collaborator

I think it is not really a problem that settings can be modified after the model has been created. However, maybe we could make them immutable once a call to propagate() or solve() has been done. But even for this, I don't really see the point (except avoiding concurrent modification).

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

3 participants