artofscience / SAOR

Sequential Approximate Optimization Repository
GNU General Public License v3.0
5 stars 1 forks source link

Move settings from `constants.py` to class attributes #15

Closed MaxvdKolk closed 3 years ago

MaxvdKolk commented 3 years ago

We might want to avoid depending global configuration, e.g. number of iterations, move limits, etc., as coming from constants.py.

It could improve the flexibility of the code when these variables are attributes of different classes, e.g. we pass explicitly the maximum iterations to an convergence criteria instance, similar for move limits etc.

If we need to set defaults in other ways, I would suggest to set these either as optional arguments value = default value in the initialisation, or if really some pseudo-global constants are needed, we might define a global variable for that specific file and assign that to the default value.

global_setting = 1

class Class:
    def __init__(self, optional_argument=global_setting)
artofscience commented 3 years ago

Imo we can fully remove constants.py, i do not see any benefits for this project

Giannis1993 commented 3 years ago

Aight! The idea behind constants.py was to have a file from which you can change the values of certain constants (e.g. move limit) and investigate its effect. I found it quite useful when I was testing different problems to play with the values of constants and have them all in one place. But for this project, I see there are only 3 constants (i.e. tolerance, move limit and max iterations), so I guess it doesn't make much sense. I shall remove the file and place those defaults somewhere else.

MaxvdKolk commented 3 years ago

I think there are different use cases for such a constants.py (or other configuration files). However, during development of the package I would suggest to avoid using global constants and place the default values as default settings in the classes or as default (optional) arguments to the classes. This ensures that if someone---once the package is public---install and uses the package, they do not need to rely on global variables.

Something that could of course be done, is to introduce the constants.py as a sort of context/config class, which is passed by default to the optimiser. Maybe something with options = Options(). So that it is easy for the user to define another set of options, simply by passing another variant of the Options class. However, as the number of constants is currently so low, I would suggest to just pass them as arguments---either individually or grouped as a dictionary


Once the layout of the package is more stable, we can think of ways to add such configuration information from a simple config file. I think IPOPT uses a ip.opt file (or something like that) that you can place in the current running directory where you start the optimisation from and IPOPT will read any configuration from that file. This might align nicely with the ability to write out all settings for purpose of reproducibility.

Giannis1993 commented 3 years ago

Removed constants.py in the dev-PROBLEM_STRUCT so I think we can close this issue. We can use #21 if we want to have different configurations/options. Ok?