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

Loggers design issues #114

Open
jdreo opened this issue Dec 20, 2021 · 1 comment
Open

Loggers design issues #114

jdreo opened this issue Dec 20, 2021 · 1 comment
Labels

Comments

@jdreo
Copy link
Contributor

jdreo commented Dec 20, 2021

The Analyzer logger takes triggers as a first parameter, which effectively forces a naive user to pass at least the on_improvement trigger for the logger to work. I would rather see the triggers defaulted near the end of the constructor parameters, so that a naive user would not have to bother with this feature (which is more for advanced users).

The Analyzer also makes a difference between Properties and Attributes, but I don't understand why. analyzer::structures::Attribute stores a key-value pair, I guess to log the algorithm's parameters. But this is also the objective of Properties, why have a specific class for Analyzer? The aim of properties is also to allow to log any algorithm parameter in any logger.

The properties were also designed so as to be a generic view about anything that would be logged. In that sense, they are also a way to control if the x are stored (or not). But loggers have a store_position flag, which necessitates additional code to check if one wants to log log_info.current.x. It would seem more logical to let a property handle this. That way, one does not need additional code, but only to pass (or not) a Property handling the current x.
However, given that a constructor with a boolean is surely easier to grasp by a newcomer, we should keep a separated —simplified— constructor with this flag, and add the related property if it is true.

Additionally, hiding the default necessary properties is a good idea, and I would suggest going further so as to do the same with triggers: have the on_improvement as hidden default, and allow for additional triggers in the full constructor.

The constructor that I would find easier to grasp for a newcomer would put the most sensible parameters first, something like:

//! Simple constructor.
Analyzer(
               const std::string &folder_name = "ioh_data",
               const std::string &algorithm_name = "algorithm_name",
               const std::string &algorithm_info = "algorithm_info",
               const fs::path &root = fs::current_path(),
               const bool store_positions = false,
               const Properties &additional_properties = {}
    ) {}
//! Constructor with full control.
Analyzer(
               const std::string &folder_name = "ioh_data",
               const std::string &algorithm_name = "algorithm_name",
               const std::string &algorithm_info = "algorithm_info",
               const fs::path &root = fs::current_path(),
               const Properties &additional_properties = {},
               const Triggers &additional_triggers = {}
    ) {}
@jacobdenobel
Copy link
Contributor

jacobdenobel commented Dec 22, 2021

@jdreo You make some valid points in this issue, and I will answer them seperatly here.

  1. Triggers should be defaulted near the end, in a similar fashion as with the Propertys. I agree, this should be something to be on the todo list for future releases
  2. analyzer::structures::Attribute should be replaced with Propertys. I would have liked to have used them for this. The thing is, the timing of when these values are logged is different. Namely, they are not logged at the sime time with the normal logging behaviour triggers and properties, at every evaluation, but rather in between runs or in between problems. These structures are therefore not functionally equavalent to what Propertys do, and are just a way of handling/grouping other metadata used in the Analyzer logger. If you think we can use properties/triggers here, I would love to hear your thoughts on it.
  3. store_position. I agree, this would be best suited with a Property, however, since Propertys now only support a single double to be logged, this wouldn't work in the current setup. The way this is implemented now, is a quick solution, in order to get the same functionality as we had before. In the future, I think it would be good formally add a to_string method to Property, allowing other data types to be logged.
  4. Update/simplify the constructor order. I agree, this can be made simpler. We can put this on the todo list.

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

2 participants