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

Convert to pip module #90

Merged
merged 24 commits into from
Dec 4, 2018
Merged

Convert to pip module #90

merged 24 commits into from
Dec 4, 2018

Conversation

BenBrostoff
Copy link
Owner

@BenBrostoff BenBrostoff commented Nov 25, 2018

This PR will address #63 . I've published a version (1.0.0) to pip. I'm putting my TODO list here so people can follow along as this gets developed:

  • - Remove all Namespace / CLI stuff
  • - Expose rules
  • - Merge with master once @sharkiteuthis 's PR is merged
  • - Add repl.it sandbox link to README

Not completed, okay to move to issues:

  • - Allow option to control flex position (blacklist or whitelist)
  • - Allow user to pass in custom functions for player pool

@sharkiteuthis - this isn't ready yet but I'm hoping to finish in the next few days. If you're working on other branches it may be useful to periodically pull from this branch. I've released 1.0.0 and even though this is a little rough around the edges, I think we should merge to master so working on feature branches is easier. The three things that are core functionality that are not in the exposed library right now are 1) uploading CSVs to DraftKings / FanDuel and 2) exposure work you completed (it's there actually but not documented or tested) and 3) pick em optimizer for DraftKings. I can create issues for all three.

I invited you to the repo so you can have review access - let me know if you didn't get it 👍

UPDATE - you can experiment with the exposed module on repl.it here

@sharkiteuthis
Copy link
Collaborator

let me know when you want me to review

@BenBrostoff
Copy link
Owner Author

Will do, this might take a few days because the API is changing in a significant way.

@BenBrostoff BenBrostoff mentioned this pull request Nov 26, 2018
@sharkiteuthis
Copy link
Collaborator

What are these items?

  • Expose rules
  • Allow option to control flex position (blacklist or whitelist)
  • Allow user to pass in custom functions for player pool

Do you mind explaining what you have in mind here? Is there overlap with #94?

@BenBrostoff
Copy link
Owner Author

BenBrostoff commented Nov 27, 2018 via email

@sharkiteuthis
Copy link
Collaborator

I like the rules idea. In general I don't stack QB/RB unless it's Dak/Zeke, Jackson/Edwards, or Cam/CMC. It would be cool to handle that with a single rule instead of having to write 29 groups

@sharkiteuthis sharkiteuthis mentioned this pull request Nov 28, 2018
@BenBrostoff BenBrostoff changed the title [WIP] Convert to pip module Convert to pip module Dec 2, 2018
Copy link
Collaborator

@sharkiteuthis sharkiteuthis left a comment

Choose a reason for hiding this comment

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

Can you create issues for:

  1. uploading CSVs to DraftKings / FanDuel and 2) exposure work you completed (it's there actually but not documented or tested) and 3) pick em optimizer for DraftKings.

You can assign (2) to me.

I'll change my RuleSet and Rule classes on my WIP branch to something like BuildRules or LineupRules to avoid confusion with the DK/FD game ruleset. That's a more accurate classname anyway.

I'm also seeing a syntax error when I run tests locally:

  File "/dk/draftfast/__init__.py", line 1, in <module>
    from draftfast import (
  File "/dk/draftfast/optimize.py", line 8, in <module>
    from draftfast.rules import RuleSet
  File "/dk/draftfast/rules.py", line 25
    'NFL': 50_000,
                ^
SyntaxError: invalid syntax

----------------------------------------------------------------------
Ran 1 test in 0.497s

Copy link
Collaborator

@sharkiteuthis sharkiteuthis left a comment

Choose a reason for hiding this comment

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

Otherwise, this lgtm, I agree that getting it into master even with a few things broken will make the rest easier

league='NHL',
roster_size='9',
position_limits=[['C', 9, 9]],
salary_max=50_000,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I get a syntax error here

}


SALARY_CAP = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

and here

@BenBrostoff
Copy link
Owner Author

@sharkiteuthis Ugh, this is a Python 3.5 thing. I'm upgrading the Dockerfile to Ubuntu 17.10 which I think uses 3.6 by default, will see if that resolves.

@BenBrostoff
Copy link
Owner Author

Also the Travis build is no longer triggering. I think this is related to them deprecating travis-ci.org... not worrying about it for now.

@BenBrostoff
Copy link
Owner Author

Okay @sharkiteuthis , once you approve this I'll merge in 👍 . Added the issues you mentioned.

@sharkiteuthis
Copy link
Collaborator

building the new docker image, numpy takes forever on my laptop

@sharkiteuthis
Copy link
Collaborator

works for me

@BenBrostoff BenBrostoff merged commit 3e186c0 into master Dec 4, 2018
@sharkiteuthis sharkiteuthis deleted the feature/pip branch July 3, 2019 11:56
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