Skip to content
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

MLB showdown #141

Merged
merged 7 commits into from
Jul 3, 2019
Merged

MLB showdown #141

merged 7 commits into from
Jul 3, 2019

Conversation

BenBrostoff
Copy link
Owner

@BenBrostoff BenBrostoff commented Jun 30, 2019

Note - work in progress

Left to do:

  • Deploy new version
  • Add documentation
  • Add repl.it showdown example

draftfast/orm.py Outdated
@@ -58,7 +58,7 @@ def __contains__(self, player):
if p.name == player or p.short_name == player:
return True
elif isinstance(player, Player):
return p in self.players
return self.players
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sharkiteuthis Not sure if you wrote this piece but assuming this change was the intent?

Copy link
Collaborator

@sharkiteuthis sharkiteuthis Jul 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__contains__() is supposed to return a boolean, I think this change will break NFL if the string comparison short circuit doesn't return true

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the issue here is p was out of scope originally. Maybe this?:

    def __contains__(self, player):
        if isinstance(player, str):
            for p in self.players:
                if p.name == player or p.short_name == player:
                    return True
        elif isinstance(player, Player):
            return player in self.players
        else:
            raise NotImplementedError

@BenBrostoff BenBrostoff marked this pull request as ready for review July 1, 2019 11:02
@@ -131,6 +133,10 @@ def get_nfl_showdown_positions(dk: bool = False, fd: bool = False) -> list:
'NHL_SHOWDOWN': [
['FLEX', 6, 6],
],
'MLB_SHOWDOWN': [
['CPT', 1, 1],
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an issue I think for all the other showdown games. It at least impacts the display if CPT isn't a constraint here - everything shows as FLEX.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, i think NHL showdowns are broken or at least non-optimal right now. Look at get_nfl_showdown_positions or NBA_SHOWDOWN rules

@@ -13,8 +13,6 @@ def __init__(self, player: Player, captain: bool = False):
self.real_pos = self.pos
self.pos = 'CPT'
self.captain = True
self.proj *= 1.5
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a change we want. I question hard-coding a 1.5 - the drawback I guess is that the rules of DraftKings are no longer "exposed" anywhere - but if we are going to expose them, it should be done as something you can pass into the initializer or maybe in RuleSet.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought per #140 the CSVs provided by DK include CPT entires?

Copy link
Owner Author

@BenBrostoff BenBrostoff Jul 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They do - I think I want to keep ShowdownPlayer though since real_pos is valuable and used by the optimizer. EDIT - so in that sense the CSV exposes the rules, but it's still a little tricky since I don't know that everybody who uses this library decides to directly ingest the CSV. I do though so it's good enough for me now :-).

@BenBrostoff BenBrostoff merged commit f126d86 into master Jul 3, 2019
@BenBrostoff BenBrostoff deleted the mlb-showdown branch July 3, 2019 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants