Chore/plot tool cleanup #79

naterush · 2017-10-26T02:38:34Z

Cleans up the plot tool just a bit. Moves the language from bets -> messages. Also, b/c we now pass in the messages labels, this should be general enough for binary as well w/out any change to the plot tool.

We can now use this display option instead of the report option in a couple places (in both the simulation runner and the testing language). I'm not totally sure how to make the migration over, though - I tried to add arguments to the parser, but was thrashing (new fav word :) ) around for a while. Something about parsing boolean command line args. Would love to get your thoughts @djrtwo

Also, the make_gif function is probably broken as of now -- but it probably can be refactored an simplified in any case.

djrtwo · 2017-10-26T03:29:45Z

simulations/simulation_runner.py

@@ -76,10 +75,14 @@ def plot(self):
        for chains in vals_chain:
            edgelist.append(utils.edge(chains, 2, 'blue', 'solid'))

+        message_lables = {}


spelling --> "labels"

djrtwo · 2017-10-26T03:41:57Z

Looks like some sanity added! :)

What arguments are you trying to add?
If it's a boolean flag, you can do something like

parser.add_argument(
    '--hide-display',
    help='Hide simulation display',
    action='store_false'
)

args.hide_display

If you use 'store_true' it'll be False unless the flag is used.
If you use 'store_false' it'll be True unless the flag is used.

I imagine this might be what you were wondering

naterush · 2017-10-27T07:20:55Z

@djrtwo Added a class BlockchainPlotTool that inherits from PlotTool. Essentially, this is the class that deals with building everything that we actually display in the case of the blockchain visualizations.

This moves us closer to being able to easily add binary, as we just have to implement a BinaryPlotTool that inherits from the plot_tool as well. It just keeps track of different things (e.g. does not worry itself with displaying any chains, etc). It might not actually necessary for these BlockchainPlotTool and BinaryPlotTool to inherit from the PlotTool - but let me know what you think.

In any case, this moves simulation runner a lot closer to being totally data-structure-agnostic. The last remaining function that isn't data structure is _check_messages_for_safety. I'm not sure what the best way to make this data-structure-agnostic, either; I think what we describe in the issue about changing check_estimate_safety works (have a last_finalized_estimate instead of block), but I can't figure out how to change how we check for estimate safety in the simulator.

One possible way of doing so it making it so we don't call check_estimate_safety with an actual estimate - we just call it and have the validator handle which estimates to check and how to check them (binary would check just the most recent estimate, and blockchain would check back to the most recent safe block).

I don't love this solution, as it will either b) have a different validator for each data structure (though we can make ) a) require us to implement something like the oracle_manager (but this just pushes the complexity and differences down to a lower level, so I doubt it's worth it).

Also, gif creation now works by default if you add a --save flag. Yay! It's super-duper-janky though (make all these files, then copy them to thumbnails [not sure why :) ], then make the GIF). If you can take a look at things that would be awesome. I'm sure there are probably 100 ways it could be simplified, maybe by using a different library or something.

naterush · 2017-10-27T07:28:43Z

Not sure if it needs to inherit from PlotTool at all. Might be totally unnecessary.

djrtwo · 2017-10-27T18:59:16Z

Inheritance on this depends on how much code is generic to plotting in general. Haven't taken a deep look to assess...

As for _check_messages_for_safety, I would offload almost the entire thing to Validator. Something like Validator.update_estimate_safety() which would maybe offload to the view (not sure). The Validator or View run the iterative check for blockchain and the more simple check for Binary.

^^ just read your next paragraph and saw you proposed this. Can we pass the work onto the view rather tahn the validator? Validator consults its View to update it's safety stuff

Not sure if that's clear. we can discuss more later

djrtwo · 2017-10-27T18:59:35Z

Let me know if/when you want me to review the codez

djrtwo · 2017-10-27T19:04:30Z

Holy shit, i just looked at BlockchainPlotTool and it's... legible :)

djrtwo · 2017-10-27T19:11:52Z

casper/network.py



 class Network:
    """Simulates a network that allows for message passing between validators."""
-    def __init__(self, validator_set):
+    def __init__(self, validator_set, display=True, save=False):


Looks like network isn't using these two new params

naterush · 2017-10-27T19:25:33Z

@djrtwo a review now would be awesome. Looking forward to hearing what ya think about the inheritance stuff vs. just importing and using the plot_tool stuff.

Also, the GIF stuff probably can be dramatically cleaned up (but it works, and maybe that's enough for now!). Thanks!

naterush · 2017-10-27T19:38:46Z

Also, having Validator.update_estimate_safety() seems good. I'm not sure if checking safety (simple binary vs iterative blockchain) belongs in the validator or the view. It seems like both make sense to some degree.

That being said, view is going to have to be implemented for both data structures. If we push the work to view, we can keep the validators totally agnostic to data structure, which would be nice :)

I'm hopping on a flight in 15 minutes. If we agree on this - I'll start implementing! Let me know @djrtwo

djrtwo · 2017-10-27T19:53:52Z

GO FOR IT!

…

On Fri, Oct 27, 2017 at 2:38 PM, Nate Rush ***@***.***> wrote: Also, having Validator.update_estimate_safety() seems good. I'm not sure if checking safety (simple binary vs iterative blockchain) belongs in the validator or the view. It seems like both make sense to some degree. That being said, view is going to have to be implemented for both data structures. If we push the work to view, we can keep the validators totally agnostic to data structure, which would be nice :) I'm hopping on a flight in 15 minutes. If we agree on this - I'll start implementing! Let me know @djrtwo <https://github.com/djrtwo> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#79 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABXf-6LBRPs42HGpHFn3y3QgEQFli8Qlks5swjFHgaJpZM4QG7bH> .

djrtwo · 2017-10-27T19:54:07Z

Did I make it in time? message said 14 minutes ago...

…

On Fri, Oct 27, 2017 at 2:53 PM, danny ryan ***@***.***> wrote: GO FOR IT! On Fri, Oct 27, 2017 at 2:38 PM, Nate Rush ***@***.***> wrote: > Also, having Validator.update_estimate_safety() seems good. I'm not sure > if checking safety (simple binary vs iterative blockchain) belongs in the > validator or the view. It seems like both make sense to some degree. > > That being said, view is going to have to be implemented for both data > structures. If we push the work to view, we can keep the validators > totally agnostic to data structure, which would be nice :) > > I'm hopping on a flight in 15 minutes. If we agree on this - I'll start > implementing! Let me know @djrtwo <https://github.com/djrtwo> > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#79 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/ABXf-6LBRPs42HGpHFn3y3QgEQFli8Qlks5swjFHgaJpZM4QG7bH> > . >

djrtwo

a handful of comments
I see what you mean in that PlotTool could be some functions you import... I think it's fine as is, and it is responsible for knowing about save_paths and report_number and stuff. It rides the line as to overdoing the OO thing.
Lint! It's much better than some past PRs but there are some low hanging fruit like unnecessary new lines and stuff.

djrtwo · 2017-10-28T13:47:30Z

casper/plot_tool.py

-        iterator += 1
-        if iterator == frame_count_limit:
-            break
+    def make_gif(self, frame_count_limit=IMAGE_LIMIT, gif_name="mygif.gif", frame_duration=0.2):


default fame_duration seems a bit too quick. I would use 0.4 or 0.5.

djrtwo · 2017-10-28T13:48:20Z

casper.py

@@ -44,18 +44,26 @@ def main():
        '--report-interval', type=int, default=config.getint("ReportInterval"),
        help='specifies the interval in rounds at which to plot results'
    )
+    parser.add_argument(


The combo of these two params makes it so vlad doesn't have to click anything for the simulations to move forward :)

Just hide-display and open up the saved gif

djrtwo · 2017-10-28T13:58:26Z

casper/blockchain_plot_tool.py

+        self.message_labels = {}
+
+
+    def update(self, message_paths=None, sent_messages=None, new_messages=None):


I like this method a lot.

djrtwo · 2017-10-28T14:06:09Z

casper/network.py

@@ -1,6 +1,6 @@
 """The network module .... """
 from casper.view import View
-import casper.plot_tool as plot_tool
+from casper.plot_tool import PlotTool


imported but not used

djrtwo · 2017-10-28T14:09:57Z

casper/plot_tool.py


-    graph = nx.Graph()
+        if save:
+            graph_path = os.path.dirname(os.path.abspath(__file__)) + '/../graphs/'


Move logic to _setup_graph_paths or something. I like to keep __init__ looking clean. It's the method you should be able to look at to get a quick high level view of the object.

djrtwo · 2017-10-28T14:10:55Z

casper/plot_tool.py

-        graph.add_edges_from([(bets, bets)])
+        self.report_number = 0
+
+        # if there isn't a graph folder, make one!


Remove notes

djrtwo · 2017-10-28T14:21:43Z

casper/plot_tool.py

+        for message in nodes:
+            if message not in message_colors:
+                node_color_map[message] = 'white'
+            else:


use elif here instead of else and then a deeper indent

djrtwo · 2017-10-28T14:33:30Z

casper/testing_language.py

-            color_mag=self.color_mag,
-            edges=edgelist
+        message_labels = {}
+        for block in self.network.global_view.messages:


We're doing a lot of the same work in BlockchainPlotTool.plot as we are doing in this report method. Can/should the test_lang offload the gathering of stuff to the plot_tool?

Good call, probably most of it can be refactored into . I'll rework this in a new PR after we get this + the other PR merged.

Cool, open up an issue so it doesn't get overlooked

Addressed!

djrtwo · 2017-10-29T00:31:23Z

casper/safety_oracles/clique_oracle.py

@@ -112,7 +112,4 @@ def check_estimate_safety(self):

            # Return the number of faults we can tolerate, which is one less


move comments over to the left

djrtwo · 2017-10-29T00:32:29Z

casper/safety_oracles/clique_oracle.py

-        # Return the number of faults we can tolerate, which is one less
-        # than the number that need to equivocate.
+            # Return the number of faults we can tolerate, which is one less
+            # than the number that need to equivocate.


naterush added 7 commits October 25, 2017 14:33

update plot tool to be more generic

bb26ec7

add functions for displaying and saving respectivly

57a337a

add functions for displaying and saving respectivly

009e7fd

further cleanup the plot_tool

e43d62c

let network take display, save as args

bbb82a3

Merge branch 'master' into chore/plot_tool_cleanup

6873291

fix a few merge conflicts

e91348e

djrtwo reviewed Oct 26, 2017

View reviewed changes

naterush added 2 commits October 25, 2017 23:38

lables -> labels

8e2b76b

update reporting for testing lang

8b14d0e

naterush mentioned this pull request Oct 26, 2017

enable blockchain and binary consensus to live in same branch #26

Closed

naterush added 4 commits October 27, 2017 01:02

move reporting out of network in prep for more changes

cf51845

add BlockchainPlotTool, being moving plotting out of sim for abstraction

2fa3194

temp fix to some sim runner tests

5dbe08d

make gif making work, still a WIP

e965a57

djrtwo reviewed Oct 27, 2017

View reviewed changes

remove unsed args from network

8c23592

naterush changed the title ~~[WIP] Chore/plot tool cleanup~~ Chore/plot tool cleanup Oct 27, 2017

naterush requested a review from djrtwo October 27, 2017 23:40

naterush mentioned this pull request Oct 27, 2017

Feat/abstraction #83

Merged

djrtwo previously requested changes Oct 28, 2017

View reviewed changes

naterush added 3 commits October 28, 2017 16:40

lints on plot tool

a03b0e5

remove unused imports

310b33b

more linting

33db3c0

naterush added 3 commits October 28, 2017 19:52

Merge branch 'master' into chore/plot_tool_cleanup

03c39eb

fix bug in the clique oracle

9e24d1f

remove print statement

b4cd5a4

djrtwo reviewed Oct 29, 2017

View reviewed changes

fixup comments

44ea09a

naterush merged commit 9cd5efa into master Oct 29, 2017

djrtwo deleted the chore/plot_tool_cleanup branch October 29, 2017 02:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chore/plot tool cleanup #79

Chore/plot tool cleanup #79

naterush commented Oct 26, 2017 •

edited

Loading

djrtwo Oct 26, 2017

djrtwo commented Oct 26, 2017

naterush commented Oct 27, 2017

naterush commented Oct 27, 2017

djrtwo commented Oct 27, 2017 •

edited

Loading

djrtwo commented Oct 27, 2017

djrtwo commented Oct 27, 2017

djrtwo Oct 27, 2017

naterush commented Oct 27, 2017

naterush commented Oct 27, 2017

djrtwo commented Oct 27, 2017 via email

djrtwo commented Oct 27, 2017 via email

djrtwo left a comment

djrtwo Oct 28, 2017

djrtwo Oct 28, 2017

djrtwo Oct 28, 2017

djrtwo Oct 28, 2017

djrtwo Oct 28, 2017

djrtwo Oct 28, 2017

djrtwo Oct 28, 2017

djrtwo Oct 28, 2017 •

edited

Loading

naterush Oct 28, 2017

djrtwo Oct 28, 2017

djrtwo Oct 29, 2017

djrtwo Oct 29, 2017

		self.message_labels = {}


		def update(self, message_paths=None, sent_messages=None, new_messages=None):

		@@ -112,7 +112,4 @@ def check_estimate_safety(self):

		# Return the number of faults we can tolerate, which is one less

Chore/plot tool cleanup #79

Chore/plot tool cleanup #79

Conversation

naterush commented Oct 26, 2017 • edited Loading

Choose a reason for hiding this comment

djrtwo commented Oct 26, 2017

naterush commented Oct 27, 2017

naterush commented Oct 27, 2017

djrtwo commented Oct 27, 2017 • edited Loading

djrtwo commented Oct 27, 2017

djrtwo commented Oct 27, 2017

Choose a reason for hiding this comment

naterush commented Oct 27, 2017

naterush commented Oct 27, 2017

djrtwo commented Oct 27, 2017 via email

djrtwo commented Oct 27, 2017 via email

djrtwo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djrtwo Oct 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naterush commented Oct 26, 2017 •

edited

Loading

djrtwo commented Oct 27, 2017 •

edited

Loading

djrtwo Oct 28, 2017 •

edited

Loading