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

Use params as a StringToStringMap for additional parameters? #197

Open
Adda0 opened this issue Mar 17, 2023 · 3 comments
Open

Use params as a StringToStringMap for additional parameters? #197

Adda0 opened this issue Mar 17, 2023 · 3 comments
Labels
For:library The issue is related to library (c++ implementation) Priority:normal Work on this sooner rather than later. Type:discussion A discussion of some particular topic in wider audience

Comments

@Adda0
Copy link
Collaborator

Adda0 commented Mar 17, 2023

We continue to use params as StringToStringMap for additional parameters for now, but we should reconsider our options and decide whether we might want to go for another approach of setting additional parameters for certain operations.

The alternative could be:

  • global configuration file,
  • global configuration class,
  • configuration class passed as an argument to the operations,
  • hierarchy of configuration classes, and optionally ad-hoc additional parameters specified by passed params argument, or
  • some other approach; remains to be suggested.
@Adda0 Adda0 added For:library The issue is related to library (c++ implementation) Type:discussion A discussion of some particular topic in wider audience Priority:normal Work on this sooner rather than later. labels Mar 17, 2023
@Adda0
Copy link
Collaborator Author

Adda0 commented Mar 17, 2023

An option suggested by @tfiedor is the following:

I think the configuration (dictionary with params), while on one hand is quite versatile, it is also quite error prone and inellegant. Maybe we could consider some different way. Maybe some singleton class Configuration? And instead of

determinize(aut, {"algorithm": "classic", "minimize": True});

We could write something like:

Config.DeteminizationAlgorithm = Determinization.Classic;
Config.MinimizeAfterDeterminization = true;
determinize(aut);

It is longer, but maybe more manageble, less error-prone (debuging "minimize": "treu" could be hell).

@martinhruska
Copy link
Member

When I see the two versions together here I prefer the original StringToStringMap. It's dynamic, flexible, extendable. I wouldn't prefere global config, at least until we have a CLI. Then it would make sense since we set Config during parsing CLI params. I would consider a common store of predefined default StringToStringMap for different algorithms where the possible options would be documented.

@Adda0
Copy link
Collaborator Author

Adda0 commented Apr 19, 2024

Discuss interface for reduce, raised by @ondrik in #406 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For:library The issue is related to library (c++ implementation) Priority:normal Work on this sooner rather than later. Type:discussion A discussion of some particular topic in wider audience
Projects
None yet
Development

No branches or pull requests

2 participants