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

Feature/base reporter #15

Draft
wants to merge 12 commits into
base: develop
Choose a base branch
from
Draft

Feature/base reporter #15

wants to merge 12 commits into from

Conversation

guimorg
Copy link
Collaborator

@guimorg guimorg commented May 28, 2020

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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

@guimorg guimorg added this to the v0.2 milestone May 28, 2020
@guimorg guimorg added the enhancement New feature or request label May 28, 2020
Comment on lines 13 to 38
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`.
"""
Copy link
Owner

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.

@guimorg
Copy link
Collaborator Author

guimorg commented May 29, 2020

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 time command to see how the observers affect the performance of the program:

# 500 Generations with (256, 256)

circle_evolution images/Mona\ Lisa\ 64.jpg --size 3 --max-generations 500  7,72s user 0,32s system 101% cpu 7,894 total - csv + logger to file 

circle_evolution images/Mona\ Lisa\ 64.jpg --size 3 --max-generations 500  7,90s user 0,25s system 101% cpu 8,005 total. -  without reporter and without matplotlib

circle_evolution images/Mona\ Lisa\ 64.jpg --size 3 --max-generations 500  10,25s user 0,44s system 100% cpu 10,590 total - csv + logger to console

# 5000 Generations with (256, 256)
No Reporter
circle_evolution images/Mona\ Lisa\ 64.jpg --size 3 --max-generations 5000  69,95s user 1,14s system 99% cpu 1:11,09 total

CSV + Logger to File
circle_evolution images/Mona\ Lisa\ 64.jpg --size 3 --max-generations 5000  87,25s user 2,39s system 99% cpu 1:30,44 total 

CSV + Logger to Console
circle_evolution images/Mona\ Lisa\ 64.jpg --size 3 --max-generations 5000  99,76s user 3,30s system 97% cpu 1:45,86 total + logger to console

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.

@guimorg guimorg self-assigned this May 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Base Reporter
2 participants