-
Notifications
You must be signed in to change notification settings - Fork 46
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
Feature/base reporter #15
base: develop
Are you sure you want to change the base?
Conversation
circle_evolution/reporter.py
Outdated
class Reporter(ABC): | ||
"""Base Reporter class. | ||
|
||
The Reporter is responsible for capturing particular events and sending | ||
them for visualization. Please, use this class if you want to implement | ||
your own Reporter. | ||
""" | ||
def __init__(self): | ||
"""Initialization calls setup to configure Reporter""" | ||
self.setup() | ||
|
||
def setup(self): | ||
"""Function for configuring the reporter. | ||
|
||
Some reporters may need configuring some internal parameters or even | ||
creating objects to warmup. This function deals with this | ||
""" | ||
|
||
@abstractmethod | ||
def update(self, report): | ||
"""Receives report from subject. | ||
|
||
This is the main function for reporting events. The Reporter receives a | ||
report and have to deal with what to do with that. For Circle-Evolution | ||
you can expect anything from strings to `numpy.array`. | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First comment
I don't like the idea of having a single function to handle all commands.
My suggestion is to add many types of functions to deal with different events:
Examples
For example, we can have an onStart method which is called when evolution is about to start.
We can have an onEnd method which is called when evolution ends.
We can have an onStop method, called when the user forcibly closes simulation.
onUpdate called when mutated species replaces the parent species.
onNextGen.... e.t.c
Second comment
I also don't like the idea of having arguments passed as many types as this kind of code is prone to errors. I suggest using a class that holds information on the report.
For example, this class can hold the current generation number, current fitness, and best specie.
Caveats
The problem with my first comment is that it can slow things down especially if using something like onNextGen... So tell me if there is a faster way to do it, or if you want to suggest something else.
The problem with my second comment is that if we pass in the species reference the programmer might modify it and it will modify the actual specie because it is only a reference. But again I believe whoever is programming should be wary, and know about this through the docs.
Conclusion
Please tell me what you think about my two comments. If you have a better idea you may propose it to me.
I've implemented something as you've proposed, two more functions that can be invoked by notifications from the Runner. I've also implemented a simple CSV reporter to keep track of the loss during evolution. I've tried using the
The main issue is when writing everything to the screen, because of that I've chosen to only log to the screen if we have improved our fitness - although that doesn't help that much - maybe printing everything to the screen is bad - I should've known. We can see that there is a difference in the long run for reporters vs. no reporters: I think that is expected, but in order to minimize this issue I am looking into removing observers from the main thread. |
Observer Pattern for Reporters and Runners
Description
I've added a Base Reporter class, in which you should provide a way to set up the reporter and deal with incoming events. I also add an Example using a LoggerReporter that only deals with string events and logs them to the screen.
To deal with Subjects I've implemented a base Runner class that already implements the observer's attach and notify functions.
Motivation and Context
Closes #12
Implements BaseReporter.
How Has This Been Tested?
Now, the CLI uses the LoggerReporter to log the generation fitness, instead of using the print function.
Screenshots (if appropriate):
Types of changes
Checklist: