You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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:
@jdreo You make some valid points in this issue, and I will answer them seperatly here.
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
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.
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.
Update/simplify the constructor order. I agree, this can be made simpler. We can put this on the todo list.
The
Analyzer
logger takes triggers as a first parameter, which effectively forces a naive user to pass at least theon_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 loglog_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:
The text was updated successfully, but these errors were encountered: