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
WIP: Refactor reporting #239
Closed
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
saulshanabrook
force-pushed
the
refactor-reporting
branch
from
July 3, 2017 21:30
5871ef8
to
e7ccc93
Compare
saulshanabrook
force-pushed
the
refactor-reporting
branch
2 times, most recently
from
July 3, 2017 22:43
d744b96
to
638bf0a
Compare
saulshanabrook
force-pushed
the
refactor-reporting
branch
2 times, most recently
from
July 4, 2017 02:52
ecab9e9
to
636d1f5
Compare
saulshanabrook
force-pushed
the
refactor-reporting
branch
from
July 4, 2017 20:16
636d1f5
to
c642016
Compare
saulshanabrook
force-pushed
the
refactor-reporting
branch
7 times, most recently
from
July 7, 2017 21:34
8183101
to
5f046d3
Compare
thelmuth
reviewed
Jul 10, 2017
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.
Just added some comments here: https://push-language.hampshire.edu/t/cleaning-up-clojush-reporting/864/17?u=thelmuth
saulshanabrook
force-pushed
the
refactor-reporting
branch
2 times, most recently
from
July 10, 2017 03:17
c6e3c0d
to
d910ba7
Compare
saulshanabrook
force-pushed
the
refactor-reporting
branch
from
July 30, 2017 04:20
cc94777
to
6586ee6
Compare
saulshanabrook
force-pushed
the
refactor-reporting
branch
from
July 31, 2017 20:53
6586ee6
to
a86fa28
Compare
Closing for now. Not sure if the code disruption is worth the gains. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
to address: https://push-language.hampshire.edu/t/cleaning-up-clojush-reporting/864
What this will improve
This changes all logging (previously called reporting) to only generate the data to log if it needs it. It also de-deduplicates the computation of various summary stats on each individual, like the mean error, number of code points, and percent parenthesis.
Scope of this pull request
This PR does not add any new features and all existing logging will behave the same. It also doesn't touch any problem specific logging, leaving that for a later change.
How does it do this?
The basic idea is that we want the statistics for each generation, and for each individual, to be a lazy map. The values of this map are dynamically computed when they are needed. We use the Plumatic's graph library to implement this. Reading that intro first will help you get a sense of how that library is used.
Another way to think about it is a form of dependency injection
What I still need to do
I am pretty sure this approach will achieve the goals, but there are likely some bugs (because of all the code changes). So I still need to:
!
and make explicitFuture improvements
If this PR is accepted, we can then add on a few other things for free.
Allow arbitrary logging of any parts of the generation.
For example, all the text reporting could be replaced with just passing in a list of paths to log and having it print out those paths. For example, we could say:
and this would log:
If this PR is accepted, changing to something like this would remove a lot of code and also simplify understanding the logs.
~~Automatic profiling of all reporting~~~ Done
We could easily enable automatic profiling of everything we compute for the logs, using the
graph/profiled
function.Expand the use of graphs for during the run as well
Having individuals be lazy-maps with dynamically generated attributes could be a nice way to represent the computation of things like
errors
or any other attribute. Also, if you check out theindividual
logging file, you can see how it pulls in stuff from the argmap to compute things. This concept could be extended for things like letting the push executor get this from the argmap.Auto documenting the types of data availableDoneWe should be able to use the Graph tools to print out all the data available for the different events. Maybe this means making Clojush a real CLI app with multiple entrypoints.