Closed kcrisman closed 9 years ago
Branch pushed to git repo; I updated commit sha1. New commits:
d002904 | Changing the documentation for LaTeX rendering |
97504ea | Changing an import to a lazy import. |
2b340dc | Fixing a broken test (due to prior merge conflict). |
020d8bf | Trying to figure out getting rid of sign and matrix |
4931761 | Revert "Trying to figure out getting rid of sign and matrix" |
215dac5 | Revert "Changing an import to a lazy import." |
582d1dc | Revert "Revert "Changing an import to a lazy import."" |
Ok most of your suggestions were quick and easy fixes. However, I wasn't able to sort out the :code:sign
and :code:matrix
issue. When I added:
del matrix
del sign
to the bottom of the catalog_...
file it no longer worked (one of the tests in normal_form_games
failed and it also just would not work iat the sage prompt. I thought perhaps this was due to the lazy_import
so I reverted some of the commits and tried again.
I've included these commits for now so that you can see what I tried (020d8bf reduces the problem to one of sage
appearing instead of matrix
and sign
which is not ideal).
I'll be sure to rebase and tidy them up with a forced push once we're happy...
Thanks again :)
I've just looked through the crystals library and in the elementary.py
file there is a line:
from sage.rings.integer import Integer
but when typing crystals.elementary.
that does not appear. I have looked through the code and can't seem to see where/how that happens (there is a particular lazy_import
line that only calls in the required things but that seems to be depreciated)...
I'm not too sure what to do with this.
That's because we don't import everything from elementary.py
into catalog_elementary.py
, just the necessary crystals (as an alias).
I promise I will look at this tomorrow (as it is 1 AM here and I need to sleep) and see what's going on.
Cool: I'll try and see where you're doing that but yeah go to sleep :D
Changed branch from u/vinceknight/catalog_of_games to u/tscrim/catalog_of_games
IMO (and that of the git community), it's okay to even good to have commits and reverts in the history to show what you tried.
Anyways, I couldn't get it to work either. I could have sworn I got this to work somewhere. So instead I opted for a slightly more painful in terms of coding by importing everything necessary within each function, but improves the interface.
I also did a bunch of cleanup of the documentation (I'm slightly surprised it compiled, but it will fix some of the indentations).
New commits:
4db9dfe | Moving imports in the catalog into the functions so they don't appear under tab completion. |
fb5b850 | Some additional cleanup. |
Replying to @tscrim:
IMO (and that of the git community), it's okay to even good to have commits and reverts in the history to show what you tried.
Cool :)
Anyways, I couldn't get it to work either. I could have sworn I got this to work somewhere. So instead I opted for a slightly more painful in terms of coding by importing everything necessary within each function, but improves the interface.
Thanks for this! :)
I also did a bunch of cleanup of the documentation (I'm slightly surprised it compiled, but it will fix some of the indentations).
Just when I thought I had figured out making documentation happy :) It compiled and actually looked just fine (I think): thanks for fixing it though :)
I've added one commit which is to add Travis to the authors list? Not sure if that's the right thing to do or not but don't want to not acknowledge anyone... I suppose in a way it could be nice to add reviewers also (as reviewers) although perhaps I'm just worrying too much about making sure everyone gets credit (your help and effort is hugely appreciated Karl).
Changed branch from u/tscrim/catalog_of_games to u/vinceknight/catalog_of_games
New commits:
5ea9d23 | Adding Travis to Author list. |
I'm hardly an author as I didn't write a single line of code and only reviewed the doc/code. If anything, Karl deserves to be listed as an author and has done far more work on this than I.
Replying to @tscrim:
I'm hardly an author as I didn't write a single line of code and only reviewed the doc/code. If anything, Karl deserves to be listed as an author and has done far more work on this than I.
Happy to do whatever everyone feels comfortable with, I thought that because you actually moved import statements around etc that you 'qualified' as being an author. Very happy to just be told what to do with this one.
Replying to @drvinceknight:
Replying to @tscrim:
I'm hardly an author as I didn't write a single line of code and only reviewed the doc/code. If anything, Karl deserves to be listed as an author and has done far more work on this than I.
Happy to do whatever everyone feels comfortable with, I thought that because you actually moved import statements around etc that you 'qualified' as being an author. Very happy to just be told what to do with this one.
I'd just remove me as an author since I'm not really one. Thanks.
Branch pushed to git repo; I updated commit sha1. New commits:
bb23b82 | Revert "Adding Travis to Author list." |
I'd just remove me as an author since I'm not really one. Thanks.
Done! Thanks again for the help :) I think that if you're happy Karl, this is good to go?
+ This can be modeled as a particular type of anti coordination game
+ following two matrices:
what happened to "using the"?
I think there is also "strategies strategies" somewhere.
I just won't have time to look at this more, but if Travis is happy with the import stuff and tab completion and it passes all tests I think I won't stand in the way, you addressed all my comments. I apologize, some things have come up that took some of the time I wanted to set aside for looking at game theory stuff thus far this summer.
FTR, I am happy with the import/tab completion.
PS - I can do the final review if more needs to be done beyond the typos mentioned by Karl-Dieter.
Branch pushed to git repo; I updated commit sha1. New commits:
c69c39a | Fixing the typos. |
Replying to @tscrim:
FTR, I am happy with the import/tab completion.
PS - I can do the final review if more needs to be done beyond the typos mentioned by Karl-Dieter.
Thanks for the offer Travis. I think this is ready to either be accepted or further reviewed :)
The error that Karl found I think is somewhat left over from when I made a mess of the merging. I think I've caught it all.
Given Karl-Dieter's comments and (IIRC) tests passed, we can set this to positive review.
Awesome thanks both!
Also:
Replying to @kcrisman:
I just won't have time to look at this more, but if Travis is happy with the import stuff and tab completion and it passes all tests I think I won't stand in the way, you addressed all my comments. I apologize, some things have come up that took some of the time I wanted to set aside for looking at game theory stuff thus far this summer.
No need to apologise, I completely understand that things get busy :)
On Arando (32-bit Linux):
sage -t --long src/sage/game_theory/catalog_normal_form_games.py
**********************************************************************
File "src/sage/game_theory/catalog_normal_form_games.py", line 800, in sage.game_theory.catalog_normal_form_games.TravellersDilemma
Failed example:
g
Expected:
Travellers dilemma - Normal Form Game with the following utilities:
{(7, 3): [5, 1], (4, 7): [1, 5], (1, 3): [5, 9], (4, 8): [0, 4],
(3, 0): [9, 5], (2, 8): [0, 4], (8, 0): [4, 0], (7, 8): [0, 4],
(5, 4): [7, 3], (0, 7): [1, 5], (5, 6): [2, 6], (2, 6): [2, 6],
(1, 6): [2, 6], (5, 1): [7, 3], (3, 7): [1, 5], (0, 3): [5, 9],
(8, 5): [4, 0], (2, 5): [3, 7], (5, 8): [0, 4], (4, 0): [8, 4],
(1, 2): [6, 10], (7, 4): [5, 1], (6, 4): [6, 2], (3, 3): [7, 7],
(2, 0): [10, 6], (8, 1): [4, 0], (7, 6): [5, 1], (4, 4): [6, 6],
(6, 3): [6, 2], (1, 5): [3, 7], (8, 8): [2, 2], (7, 2): [5, 1],
(3, 6): [2, 6], (2, 2): [8, 8], (7, 7): [3, 3], (5, 7): [1, 5],
(5, 3): [7, 3], (4, 1): [8, 4], (1, 1): [9, 9], (2, 7): [1, 5],
(3, 2): [9, 5], (0, 0): [10, 10], (6, 6): [4, 4], (5, 0): [7, 3],
(7, 1): [5, 1], (4, 5): [3, 7], (0, 4): [4, 8], (5, 5): [5, 5],
(1, 4): [4, 8], (6, 0): [6, 2], (7, 5): [5, 1], (2, 3): [5, 9],
(2, 1): [10, 6], (8, 7): [4, 0], (6, 8): [0, 4], (4, 2): [8, 4],
(1, 0): [11, 7], (0, 8): [0, 4], (6, 5): [6, 2], (3, 5): [3, 7],
(0, 1): [7, 11], (8, 3): [4, 0], (7, 0): [5, 1], (4, 6): [2, 6],
(6, 7): [1, 5], (8, 6): [4, 0], (5, 2): [7, 3], (6, 1): [6, 2],
(3, 1): [9, 5], (8, 2): [4, 0], (2, 4): [4, 8], (3, 8): [0, 4],
(0, 6): [2, 6], (1, 8): [0, 4], (6, 2): [6, 2], (4, 3): [8, 4],
(1, 7): [1, 5], (0, 5): [3, 7], (3, 4): [4, 8], (0, 2): [6, 10],
(8, 4): [4, 0]}
Got:
Travellers dilemma - Normal Form Game with the following utilities: {(7, 3): [5, 1], (4, 7): [1, 5], (1, 3): [5, 9], (4, 8): [0, 4], (3, 0): [9, 5], (2, 8): [0, 4], (8, 0): [4, 0], (7, 8): [0, 4], (5, 4): [7, 3], (0, 7): [1, 5], (5, 6): [2, 6], (2, 6): [2, 6], (1, 6): [2, 6], (5, 1): [7, 3], (3, 7): [1, 5], (0, 3): [5, 9], (8, 5): [4, 0], (2, 5): [3, 7], (5, 8): [0, 4], (4, 0): [8, 4], (1, 2): [6, 10], (7, 4): [5, 1], (6, 4): [6, 2], (3, 3): [7, 7], (0, 6): [2, 6], (8, 1): [4, 0], (7, 6): [5, 1], (4, 4): [6, 6], (6, 3): [6, 2], (1, 5): [3, 7], (8, 8): [2, 2], (7, 2): [5, 1], (3, 6): [2, 6], (2, 2): [8, 8], (7, 7): [3, 3], (5, 7): [1, 5], (5, 3): [7, 3], (4, 1): [8, 4], (1, 1): [9, 9], (2, 7): [1, 5], (3, 2): [9, 5], (0, 0): [10, 10], (6, 6): [4, 4], (5, 0): [7, 3], (7, 1): [5, 1], (4, 5): [3, 7], (0, 4): [4, 8], (5, 5): [5, 5], (1, 4): [4, 8], (6, 0): [6, 2], (7, 5): [5, 1], (2, 3): [5, 9], (2, 1): [10, 6], (8, 7): [4, 0], (6, 8): [0, 4], (4, 2): [8, 4], (1, 0): [11, 7], (0, 8): [0, 4], (6, 5): [6, 2], (3, 5): [3, 7], (0, 1): [7, 11], (8, 3): [4, 0], (7, 0): [5, 1], (4, 6): [2, 6], (6, 7): [1, 5], (5, 2): [7, 3], (6, 1): [6, 2], (3, 1): [9, 5], (8, 2): [4, 0], (2, 4): [4, 8], (3, 8): [0, 4], (2, 0): [10, 6], (1, 8): [0, 4], (6, 2): [6, 2], (4, 3): [8, 4], (1, 7): [1, 5], (8, 6): [4, 0], (0, 5): [3, 7], (3, 4): [4, 8], (0, 2): [6, 10], (8, 4): [4, 0]}
**********************************************************************
1 item had failures:
1 of 6 in sage.game_theory.catalog_normal_form_games.TravellersDilemma
[58 tests, 1 failure, 0.63 s]
Changed reviewer from Karl-Dieter Crisman to Karl-Dieter Crisman, Travis Scrimshaw
So the issue is that the output order is not unique because it is from a python dict. The usual response is to run sorted(obj)
where obj
is some object which represents the desired output. The other thing you could do (either with the previous or just this) is
Travellers dilemma - Normal Form Game with the following utilities: ...
Branch pushed to git repo; I updated commit sha1. New commits:
31ec186 | Fixing the tests for the ordering of the dictionary |
Thanks Travis, I remember this being a pain with matching games :)
I have fixed this: tests pass on two different machines but it is possible that I've missed one BUT I did not go with your suggestion.
I have thrown in:
Travellers dilemma - Normal Form Game with the following utilities: ...
but then I also built the expected dictionary and just checked that the equality test between the game and the dictionary returned True
Using sorted(g)
would only return the keys of the dictionary which could potentially hide an error...
I think this is probably the best way to do this but please let me know if you think it should be reworked...
I think this solution is fine, though
+ sage: g = game_theory.normal_form_games.CoordinationGame(A=9,
+ ....: a=6,
+ ....: B=0,
+ ....: b=1,
+ ....: C=2,
+ ....: c=10,
+ ....: D=4,
+ ....: d=11)
starts looking a little longish. Was there a reason for doing that uniformly? You may also want to (briefly) mention somewhere convenient why you're doing this, or perhaps just where the utility dictionary is ==
to the game, which I wasn't expecting (since it's not a game, just a random dictionary) but is fine, as far as I care about it.
Replying to @kcrisman:
starts looking a little longish. Was there a reason for doing that uniformly?
Do you mean the multilines? That's mainly to stick with the 79 character limit, I noticed this for the larger games and then thought I should do it throughout. I agree that it looks pretty bad. I'm happy to leave as is (so that it fits with the 79 characters strictly) but perhaps the rule could be broken for some of the smaller dicts?
You may also want to (briefly) mention somewhere convenient why you're doing this, or perhaps just where the utility dictionary is
==
to the game, which I wasn't expecting (since it's not a game, just a random dictionary) but is fine, as far as I care about it.
So perhaps mention something a long the lines of:
sage: # Building a dictionary to test the required result
sage: # The order of the dictionary might differ on a given machine
sage: d = {...
sage: d == g
Let me know if that's what you mean?
Thanks again for your time with this Karl.
starts looking a little longish. Was there a reason for doing that uniformly?
Do you mean the multilines? That's mainly to stick with the 79 character limit, I noticed this for the larger games and then thought I should do it throughout. I agree that it looks pretty bad. I'm happy to leave as is (so that it fits with the 79 characters strictly) but perhaps the rule could be broken for some of the smaller dicts?
I think only do it if you really go well beyond the limit. Or just continue the multiline in such a way that there are TWO lines and not EIGHT lines :)
You may also want to (briefly) mention somewhere convenient why you're doing this, or perhaps just where the utility dictionary is
==
to the game, which I wasn't expecting (since it's not a game, just a random dictionary) but is fine, as far as I care about it.So perhaps mention something a long the lines of:
sage: # Building a dictionary to test the required result sage: # The order of the dictionary might differ on a given machine sage: d = {... sage: d == g
Oh, not even that much - maybe just before the very first instance of this, preferably not within a specific function but at the module level, something like
We can construct a game::
sage: construct a game
sage: do something with it
When we test whether the game is actually the one in question, sometimes we will build a dictionary to test it, since this can be platform-dependent, like so::
sage: d = {...
sage: d == g
True
And then the others are tacit.
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
4210a63 | Making the previous PEP8 adjustment look ok |
Ok Karl, I think this is ready now :) I've made the adjustments I believe you are suggesting :)
Presumably irrelevant remarks:
+We can then immediately obtain the Nash equilibria for the game::
Of course, in that example there is only an equilibrium, though in general presumably not.
modelled
Dang Brits.
PEP8 looks fine. If this passes tests please set to positive review, unfortunately I have something else I need to do now.
Branch pushed to git repo; I updated commit sha1. New commits:
2c111e2 | Minor tweaks |
Replying to @kcrisman:
Dang Brits.
Hehe :) My last commit addresses those two minor points.
PEP8 looks fine. If this passes tests please set to positive review, unfortunately I have something else I need to do now.
Cool: tests pass and docs build so am setting to positive review.
Sorry for not responding sooner, I've been busy moving out of my house and having to choose my time (and the past 17 hours, been traveling from SF to Korea). Here are my thoughts, not that they really count for much.
I try and keep to the 80 char/line limit, and what Vince did initially is the "best" practice according to PEP8. However, I side with Karl-Dieter on this in that it just looks ugly and it's worth it to consolidate the data and go past 80 char/line limit when it is easier to read.
However I do have an issue with this sentence:
+When we test whether the game is actually the one in question, sometimes we will
+build a dictionary to test it, since this can be platform-dependent, like so::
What is "this"? In particular, when I first read this, to me it seemed like the actual output could be different on different machines, whereas it is only the print representation of the dictionary used to model the game. Your change doesn't have to be as verbose as that (IMO), just change "this" to "the printed representation" or something like that.
Once you make that change, you can set this back to positive review on my behalf.
Replying to @tscrim:
However I do have an issue with this sentence:
+When we test whether the game is actually the one in question, sometimes we will +build a dictionary to test it, since this can be platform-dependent, like so::
What is "this"? In particular, when I first read this, to me it seemed like the actual output could be different on different machines, whereas it is only the print representation of the dictionary used to model the game. Your change doesn't have to be as verbose as that (IMO), just change "this" to "the printed representation" or something like that.
Excellent point. Will change that asap. Thanks!
I hope your move went well!
Branch pushed to git repo; I updated commit sha1. New commits:
6ec7195 | Clarifying text about testing dictionaries. |
Changed branch from u/vinceknight/catalog_of_games to 6ec7195
Changed author from Vincent Knight, James Campbell to Vince Knight, James Campbell
In #16333 comment:13, the idea arises of having built-in normal form games for pedagogical (or other) purposes, similarly to in our discrete math areas.
There are quite a few such games out there in the literature that would be assumed as 'standard'.
CC: @drvinceknight @nathanncohen
Component: game theory
Keywords: days64
Author: Vince Knight, James Campbell
Branch:
6ec7195
Reviewer: Karl-Dieter Crisman, Travis Scrimshaw
Issue created by migration from https://trac.sagemath.org/ticket/17392