-
Notifications
You must be signed in to change notification settings - Fork 147
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
Stats distribution on Football GM fixed #306
Stats distribution on Football GM fixed #306
Conversation
-Added menus to the stats distribution in Football-GM
src/worker/views/teamStatDists.ts
Outdated
@@ -1,6 +1,8 @@ | |||
import { idb } from "../db"; | |||
import { g, helpers } from "../util"; | |||
import type { UpdateEvents, ViewInput } from "../../common/types"; | |||
import { TEAM_STATS_DIST_TABLE } from "../../common"; |
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.
Since this is the only place importing it, and that probably will not change, you should just inline this below rather than put it in common.
Even better might be to make this use TEAM_STATS_TABLES with a dropdown to select the table, just like you did in PlayerStatDists. But don't feel obligated to do that... even with just a table of team stats, it's better than nothing!
src/worker/views/playerStatDists.ts
Outdated
"pts", | ||
"per", | ||
]; | ||
console.log(inputs); |
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.
Remove console.log
src/worker/views/playerStatDists.ts
Outdated
]; | ||
console.log(inputs); | ||
const stats = | ||
process.env.SPORT === "basketball" |
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.
Same thing could be done for basketball, using PLAYER_STATS_TABLES and a dropdown... but like I said in my other comment, consider that optional, cause this PR is an improvement even without that!
|
||
players = await idb.getCopies.playersPlus(players, { | ||
ratings: ["skills"], | ||
stats, | ||
season: inputs.season, | ||
}); | ||
if (process.env.SPORT == "football") { | ||
const statTable = PLAYER_STATS_TABLES[inputs.statType]; | ||
const onlyShowIf = statTable.onlyShowIf as string[]; |
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.
Is casting to string[]
really needed here?
This is cool! Left a few small comments, but I definitely want to merge this. I think when doing the original football port I ran into the same issue, where it's less clear which players to include in the distribution for football than for basketball... and then I guess I just forgot about it lol |
Still work in progress since maybe it would be neccesary to add the stats from 2018-20 to this stats too
Thanks! I added a couple more commits to hide non-numeric stats (like qbRec) and to use nice text for the variable names (like table headers in stats tables). |
Ok, so I fixed this by adding a menu item to the stats distribution when in Football GM. The only problem I found is that in some stats in Football GM like defInt the median, first and second quartile are 0 so the boxplot is not really informative. I used the same filter that you use for deciding which players has a table for each category (with the PLAYER_STATS_TABLE). I don´t know if this will be commited but it was fun anyway to fix it.

-Added menus to the stats distribution in Football-GM