Add show-match-scores command #17

WillB97 · 2021-04-27T13:02:20Z

Prints ranking. league and match points for every match.
Matches can be filtered to a single team.

Output format is:

$ srcomp show-match-scores dummy-comp HRS
                      |        Zone 0        |        Zone 1        |
 Display Name | Arena | TLA |Rank|Game|League| TLA |Rank|Game|League|
   Match 5    |   A   | BGS |  1 | 10 |    8 | HRS |  4 |  1 |    2 |
   Match 10   |   B   | SCC |  2 |  6 |    6 | DSF |  1 |  8 |    8 |
   Match 16   |   A   | QEH |  1 |  7 |    8 | HRS |  4 |  3 |    2 |
...

Prints ranking. league and match points for every match. Matches can be filtered to a single team.

PeterJCLaw · 2021-04-27T20:10:28Z

sr/comp/cli/show_match_scores.py

+    corners: Dict[int, MatchCorner]
+
+
+def collect_match_info(comp, match) -> MatchResult:


I expect mypy wants the arguments to be typed too.

I've added types in e0a900a but I have a type error with the degroup command.

sr/comp/cli/show_match_scores.py:47: error: Argument 1 to "degroup" has incompatible type "Mapping[RankedPosition, Set[TLA]]"; expected "Mapping[Optional[LeaguePosition], Iterable[TLA]]" [arg-type]

sr/comp/cli/show_match_scores.py

PeterJCLaw · 2021-04-27T20:27:58Z

sr/comp/cli/show_match_scores.py

+    print_col(match.display_name.center(name_width))
+    print_col(match.arena.center(arena_name_width))
+
+    for corner in match.corners.values():


Feels like this is going to get pretty wide even just for a four zone match. Is this the best layout for a CLI tool?

With the dummy comp the table was 120 wide, this increased to 146 when I moved to using tabulate so I've made it wrap each match at 2 zones in 52931ce which gives a width of 100.

At this point the only part that effects the width of the table is the match and arena names. The next version of tabulate looks to be adding and option to force wrap certain columns at a set width.

Alternatively if we can come up with a way to merge the game and league columns they are limited by the length of the heading. In fact omitting all the headers reduces the width to 78.

+-----------------------+------+------+------+------+--------+------+------+------+------+--------+ | Match | Zone | TLA | Rank | Game | League | Zone | TLA | Rank | Game | League | +-----------------------+------+------+------+------+--------+------+------+------+------+--------+ | Match 0 in A | 0 | | | | | 1 | CLY | 1 | 9 | 8 | | | 2 | TTN | 2 | 6 | 6 | 3 | | | | | | Match 0 in B | 0 | GRS | 1 | 5 | 8 | 1 | QMC | 2 | 3 | 6 | | | 2 | | | | | 3 | | | | | | Match 1 in A | 0 | WYC | 4 | 3 | 2 | 1 | QMS | 2 | 7 | 6 | | | 2 | LSS | 1 | 10 | 8 | 3 | EMM | 3 | 5 | 4 | | Match 1 in B | 0 | BPV | 2 | 6 | 6 | 1 | BDF | 4 | 2 | 2 | | | 2 | NHS | 1 | 7 | 8 | 3 | MEA | 3 | 4 | 4 | | Quarter 1 (#123) in A | 0 | ??? | ?? | ?? | ?? | 1 | ??? | ?? | ?? | ?? | | | 2 | ??? | ?? | ?? | ?? | 3 | ??? | ?? | ?? | ?? | | Quarter 2 (#124) in A | 0 | ??? | ?? | ?? | ?? | 1 | ??? | ?? | ?? | ?? | | | 2 | ??? | ?? | ?? | ?? | 3 | ??? | ?? | ?? | ?? | | Quarter 3 (#125) in A | 0 | ??? | ?? | ?? | ?? | 1 | ??? | ?? | ?? | ?? | | | 2 | ??? | ?? | ?? | ?? | 3 | ??? | ?? | ?? | ?? | | Quarter 4 (#126) in A | 0 | ??? | ?? | ?? | ?? | 1 | ??? | ?? | ?? | ?? | | | 2 | ??? | ?? | ?? | ?? | 3 | ??? | ?? | ?? | ?? |

Hrm. I definitely think we shouldn't mix wrapping with putting things next to each other -- so if we're having a line per zone we should always have that. Both in the sense that the output shouldn't mix the two styles, but also that we shouldn't try to guess which style to use. I can maybe see an argument for letting the user choose, however in that case I think we should ignore the width that that ends up being (since that's then the user's problem -- they got what they asked for).

I think for a first pass here we should go with a simple approach which would always work. This probably means roughly the style you've suggested here, but only one zone wide. (I think this would also simplify the code a bit?)

I think I'd also push the arena name back into its own column? Not sure how that would look in the two-arena case though.

PeterJCLaw

Thanks for putting this together, this looks like it could be really useful.
I suggest we work through defining a layout which is going to work for at least four-zone matches before making any other changes as that's likely to need the most change.
If possible I'd like the output to fit within a standard terminal -- 80 characters wide, maybe 100 if we really can't make that work. It feels like it ought to be possible to make that work even for arbitrarily many zones.

The table now wraps corners at 2, displaying them on the following line. With dummy-comp thins produces a table width of 100.

PeterJCLaw · 2021-05-08T11:22:22Z

sr/comp/cli/show_match_scores.py

+def match_index(matches: List[MatchSlot], match_num: int) -> int:
+    "Returns the index of the first slot that contains the given match number"
+    for idx, slots in enumerate(matches):
+        for match in slots.values():
+            if match.num == match_num:
+                return idx
+
+    # if no match is found use the last one
+    return idx


This shouldn't be necessary -- it should be as simple as return num since matches[num] should contain matches with that number. If that's not holding then I think we have serious issues elsewhere in SRComp.

PeterJCLaw · 2021-05-08T11:24:10Z

sr/comp/cli/show_match_scores.py

+        displayed_heading.append("Zone")
+        displayed_heading.append("TLA")
+        displayed_heading.append("Rank")
+        displayed_heading.append("Game")
+        displayed_heading.append("League")


This might be simpler/clearer using .extend or +=? (The same applies within generate_displayed_match)

Suggested change

displayed_heading.append("Zone")

displayed_heading.append("TLA")

displayed_heading.append("Rank")

displayed_heading.append("Game")

displayed_heading.append("League")

displayed_heading += ("Zone", "TLA", "Rank", "Game", "League")

Having done that we can also observe a further simplification:

zone_headers = ("Zone", "TLA", "Rank", "Game", "League") return ["Match"] + zone_headers * num_corners

PeterJCLaw · 2021-05-08T11:27:59Z

sr/comp/cli/show_match_scores.py

+            displayed_corner.append("??" if ranking is None else str(ranking))
+            displayed_corner.append("??" if game is None else str(game))
+            displayed_corner.append("??" if league is None else str(league))


I suggest maybe using - rather than ?? for unknown score values? It still conveys the unknown nature while being less attention grabbing.

??? (well, technically we should use the UNKNOWABLE_TEAM symbol) should still be used for unknown TLAs though as that's an existing SRComp standard (and is chosen to both visually look like a TLA and indicate the unknowable nature of the value).

PeterJCLaw · 2021-05-08T11:31:51Z

sr/comp/cli/show_match_scores.py

+            print('TLA not found')
+            return


Since this is an error case, please could we:

include the "bad" data in the message,

put the error message on stderr, and

return an error code so that (scripted) callers know something went wrong

I think we have some handlers for this sort of thing in the deploy script if you want some inspiration.
Aside: I've long been meaning to formalise this sort of thing though, likely styled after Django's CommandError exception, though that's likely overkill to introduce in this PR.

One approach here might be:

Suggested change

print('TLA not found')

return

exit(f"TLA {filter_tla!r} not recognised")

PeterJCLaw · 2021-05-08T11:33:22Z

sr/comp/cli/show_match_scores.py

+    parser.add_argument(
+        '--all',
+        action='store_true',
+        help="show all matches (overrides --limit)",
+    )
+    parser.add_argument(
+        '--limit',
+        default=15,
+        help="how many recently scored matches to show (default: %(default)s)",
+    )


I believe argparse has a way to express mutually exclusive options; please could we use that here?

PeterJCLaw · 2021-05-08T11:42:48Z

As a general thought -- I'd probably have this tool just not output anything for the matches which haven't been scored yet. That's how https://studentrobotics.org/comp/points behaves and I'm assuming roughly what you're aiming this to be equivalent to?
This approach avoids needing to work out what should happen to the knockout matches when filtering by TLA, which is a nice way to avoid some complexity.

PeterJCLaw · 2022-03-12T19:00:05Z

For when we come back to this -- PeterJCLaw/srcomp#13 likely makes some of the score lookup which is needed here somewhat simpler.

WillB97 added 9 commits April 26, 2021 18:53

Add show-match-scores command

f2333f8

Prints ranking. league and match points for every match. Matches can be filtered to a single team.

Make filter condition more clear

9f4f575

Merge column width calculation loops

ce5c548

Use better method for calculating number of corners

3439ad5

Imporve line formatting

e8c5ba7

Use srcomp types

1c7a01f

Remove failing import

e0ce60b

Remove unused constants

5de19d0

Add handling for empty corners

7a11ba1

PeterJCLaw reviewed Apr 27, 2021

View reviewed changes

sr/comp/cli/show_match_scores.py Outdated Show resolved Hide resolved

PeterJCLaw reviewed Apr 27, 2021

View reviewed changes

sr/comp/cli/show_match_scores.py Outdated Show resolved Hide resolved

PeterJCLaw reviewed Apr 27, 2021

View reviewed changes

sr/comp/cli/show_match_scores.py Outdated Show resolved Hide resolved

PeterJCLaw reviewed Apr 27, 2021

View reviewed changes

sr/comp/cli/show_match_scores.py Outdated Show resolved Hide resolved

PeterJCLaw reviewed Apr 27, 2021

View reviewed changes

sr/comp/cli/show_match_scores.py Outdated Show resolved Hide resolved

PeterJCLaw reviewed Apr 27, 2021

View reviewed changes

sr/comp/cli/show_match_scores.py Outdated Show resolved Hide resolved

PeterJCLaw reviewed Apr 27, 2021

View reviewed changes

WillB97 added 12 commits May 3, 2021 20:04

Correct typo

4d06c95

is None

8afa4be

Remove unneeded getattr

07f2592

Use Optional types instead of Union

14e0af3

Solve linting and typing errors

be28fd7

Use tabulate for table display

8bde289

Make the table more compact

52931ce

The table now wraps corners at 2, displaying them on the following line. With dummy-comp thins produces a table width of 100.

Add option to limit output to a number of recent matches

fbbb3bc

Add some comments

46cc13d

Linting

b5d4c8d

Add typing

e0a900a

Validate TLA

c682c2e

WillB97 requested a review from PeterJCLaw May 4, 2021 13:01

PeterJCLaw reviewed May 8, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add show-match-scores command #17

Add show-match-scores command #17

WillB97 commented Apr 27, 2021

PeterJCLaw Apr 27, 2021

WillB97 May 4, 2021

PeterJCLaw Apr 27, 2021

WillB97 May 4, 2021

PeterJCLaw May 8, 2021

PeterJCLaw left a comment

PeterJCLaw May 8, 2021

PeterJCLaw May 8, 2021 •

edited

Loading

PeterJCLaw May 8, 2021 •

edited

Loading

PeterJCLaw May 8, 2021

PeterJCLaw May 8, 2021

PeterJCLaw commented May 8, 2021

PeterJCLaw commented Mar 12, 2022

		corners: Dict[int, MatchCorner]


		def collect_match_info(comp, match) -> MatchResult:

	print('TLA not found')
	return
	exit(f"TLA {filter_tla!r} not recognised")

Add show-match-scores command #17

Are you sure you want to change the base?

Add show-match-scores command #17

Conversation

WillB97 commented Apr 27, 2021

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

PeterJCLaw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PeterJCLaw May 8, 2021 • edited Loading

Choose a reason for hiding this comment

PeterJCLaw May 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PeterJCLaw commented May 8, 2021

PeterJCLaw commented Mar 12, 2022

PeterJCLaw May 8, 2021 •

edited

Loading

PeterJCLaw May 8, 2021 •

edited

Loading