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.

Sign up for GitHub

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

Jump to bottom

Initial file structure changes for Package #16

Merged
merged 7 commits into from
Jun 8, 2021
Merged

Initial file structure changes for Package #16

merged 7 commits into from
Jun 8, 2021

Conversation

JuiP
Copy link
Contributor

@JuiP JuiP commented Feb 26, 2021

Fixes #13 @jackgerrits
Now, it can be installed as: python3 -m pip install vw-estimators==1.0.0
Do let me know if any changes are needed. Thanks!

JuiP
JuiP commented Feb 26, 2021
View reviewed changes
setup.py Outdated Show resolved Hide resolved
@jackgerrits
Copy link
Member

Hi @JuiP,

Thanks for taking a look at the issue, I appreciate the work on the codebase. However, this issue was just about implementing the setup.py script and not actually releasing the code to PyPi. More work is certainly needed before this repo is ready for release. Especially a 1.0.0 release (we would be in 0.x prerelease for some time). Would it be okay if you could remove the vw-estimators package off of PyPi?

@JuiP
Copy link
Contributor Author

JuiP commented Feb 27, 2021 •
edited
Loading

@jackgerrits

Apologies for misunderstanding the issue. The package has been removed off of PyPi and the version number in the setup file has been updated to "0.0.1" in the commit cadf3c2.

I see that #10 has major additions and has been open for quite some time now.

Thanks!

@cheng-tan cheng-tan mentioned this pull request May 11, 2021
Open
@JuiP JuiP changed the base branch from master to rlos2021_cfe May 12, 2021 20:19
@ataymano
Copy link
Member

ataymano commented May 24, 2021 •
edited
Loading

Hi @JuiP ,
Looks like right now all code in this repo can roughly be splitted into several categories:

  • esimators themselves (i.e. ips, mle, ...). All of them seem to be implemented as contextual bandits ones, but at the same time we are going to introduce estimators for other type of problems - for sure conditional contextual bandits + maybe cats (which is right now is implemented as transformer to cb data). Maybe we could have package per problem type (only cb for now)?
  • helpers / utils - looks like right now we have only ds_parse.py from this category. Probably it should be either completely removed or renamed to something from utils.py in future, but it is probably ok to leave it as is for now
  • executable scripts - looks like it is basic_isage.py. If we think about estimators library as a library, this one is actually just example of possible usage, so it should not be part of the pip package and should stay either on level above or in separate examples/scripts folder.

ataymano
ataymano reviewed Jun 2, 2021
View reviewed changes
Copy link
Member

@ataymano ataymano left a comment

Choose a reason for hiding this comment

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

Pseudo inverse is estimator for slates , not for conditional contextual bandits.

@JuiP
Copy link
Contributor Author

JuiP commented Jun 2, 2021

The documentation here suggested that slates builds upon ccb...

Pseudo inverse is estimator for slates, not for conditional contextual bandits.

So then what would go under conditional contextual bandits? Also, do I rename the folder to slates?

@ataymano
Copy link
Member

ataymano commented Jun 3, 2021

The documentation here suggested that slates builds upon ccb...

Pseudo inverse is estimator for slates, not for conditional contextual bandits.

So then what would go under conditional contextual bandits? Also, do I rename the folder to slates?

Vw is built in the way that all problems are implemented as reductions to other (implemented before) problems.
So, conditional contextual bandits is implemented as reduction to contextual bandits and slates is implemented as reduction to conditional contextual bandits.
But from the interface point of view, these 3 problems are different:

  1. Estimators for CB: taking (p_log:float, p_pred:float, r:float)
  2. Estimators for CCB: taking (p_log:list[float], p_pred:list[float], r:list[float])
  3. Estimators for slates: taking (p_log:list[float], p_pred:list[float], r:float)

Right now there is no CCB estimators implemented in this repo, so I suggest to move pseudo_inverse to slates folder, and conditional contextual bandits can be removed for now.

@ataymano
Copy link
Member

ataymano commented Jun 3, 2021

Also, there is a convention in python package/module naming:
https://www.python.org/dev/peps/pep-0008/#package-and-module-names
which is violated if we name them ContextualBandits / ConditionalContextualBandits / Slates.
Can we name them either contextual_bandits / slates or cb / slates?

@JuiP
Copy link
Contributor Author

JuiP commented Jun 4, 2021

The documentation here suggested that slates builds upon ccb...

Pseudo inverse is estimator for slates, not for conditional contextual bandits.

So then what would go under conditional contextual bandits? Also, do I rename the folder to slates?

Vw is built in the way that all problems are implemented as reductions to other (implemented before) problems.
So, conditional contextual bandits is implemented as reduction to contextual bandits and slates is implemented as reduction to conditional contextual bandits.
But from the interface point of view, these 3 problems are different:

  1. Estimators for CB: taking (p_log:float, p_pred:float, r:float)
  2. Estimators for CCB: taking (p_log:list[float], p_pred:list[float], r:list[float])
  3. Estimators for slates: taking (p_log:list[float], p_pred:list[float], r:float)

Right now there is no CCB estimators implemented in this repo, so I suggest to move pseudo_inverse to slates folder, and conditional contextual bandits can be removed for now.

Got it! Thank you so much for the clarification! 😁

ataymano
ataymano approved these changes Jun 5, 2021
View reviewed changes
Copy link
Member

@ataymano ataymano left a comment

Choose a reason for hiding this comment

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

Looks good to me. But let's rename PR to something like "Package file structure update" before merge - otherwise it looks a bit confusing

@JuiP JuiP changed the title Make the library a python package Initial file structure changes for Package Jun 5, 2021
@JuiP
Copy link
Contributor Author

JuiP commented Jun 5, 2021

@ataymano Agreed! I have changed the title of this PR

@ataymano ataymano requested a review from cheng-tan June 5, 2021 14:55
@ataymano
Copy link
Member

ataymano commented Jun 5, 2021

Great, thank you! @cheng-tan, can you please take a look as well?

cheng-tan
cheng-tan reviewed Jun 7, 2021
View reviewed changes
setup.py Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@cheng-tan cheng-tan merged commit 144091b into VowpalWabbit:rlos2021_cfe Jun 8, 2021
ataymano added a commit that referenced this pull request Feb 14, 2022
@JuiP @ataymano
Merge branch rlos2021_cfe to master (#36)
bb971f6 
* Initial file structure changes for Package (#16)

* Interface Approach2 (#29)

* Split ips and snips. Make two classes: Estimator and Interval

* Split pseudo_inverse into 2 classes: Estimator and Interval

* Fix test_pi, remove redundant file ips_snips, Remove type argument from get_estimate

* Slates interface implementation

* Cb interface initial commit

* Rename file and change class names

* Edit doc strings

* Change count datatype to float in cb_base

* Added gaussian, clopper_pearson files and removed type from cb interface

* Add newline at the end of file

* Changes for slates

- Renamed file from slates_helper to slates_base
- Added gaussian.py
- Removed type from get_interval
- Removed type from get_estimate
- Change doc strings for the slates interface
- Changed class names
- Changed data type of count
- Fixed data type of p_log and p_pred
- Removed unused imports

* Remove redundant imports and code

* Change method name to get()

* Rename file to base and change class name of ips, snips

* Change doc strings and variable name: slates

* Changes for test_pi

* Cressieread Interval update

* Changes folder name and class names (#31)

* Minimal changes tobasic-usage (#32)

* Improvements for setup.py and slates (#33)

* imports fix (#34)

* Adding Tests (#35)

* Unit tests added

* Test for multiple examples

* Added test for checking narrowing intervals

* Combine all unit test functions into one

* Added comments

* Added another example generator

* Fixed Imports

* Change variable names and fix typo

* Added check for correct format of Confidence Interval

* Separate bandit and slates tests

* Move functions to utils

* Added test for correctness(slates)

* Comments added for test_bandits

* Added tests for slates intervals

* Move data generators from helper files to test_* files

* Remove num_slots as a parameter in util functions

* Combine run_estimator function

* Combine SlatesHelper and BanditsHelper

* Move assert statements from run_estimator() to test_*.py

* Move assert statements from Helper() functions to test_*.py file

* Improving code consistency

* Defined static methods and renamed file to utils.py

* Add function assert_is_close to utils

* Variable name changed

* Restructuring of code

* CI improvements (#38)

* Added support for Python version 3.9

* CI: Check test coverage

* Added interface and module for ccb (#37)

* Added ccb estimator (#39)

* Added ccb estimator file

* Removed type and added Interval()

* Added unit test for ccb + code corrections in ccb.py

* Test for correctness and narrowing Intervals added

* Changed module name

* Change variable name

* Removed hard coding for specific alpha values in gaussain files (#44)

* Add tests (#43)

* use random.seed() to make test scenarios reproducible

* Change function names

* Rename variables

* Rename variables listofestimators->estimators and listofintervals->intervals

* Renamed variables for test_narrowing_intervals

* Added test to check alpha value is not hardcoded for bandits

* Renamed to test_different_alpha_CI

* Rlos2021 minor cleanup (#45)

* minor cleanups

* py35 removal

* more type hints

* snake case

* ValueError

* snake case

Co-authored-by: Alexey Taymanov <41013086+ataymano@users.noreply.github.com>
Co-authored-by: Alexey Taymanov <ataymano@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cheng-tan cheng-tan cheng-tan left review comments

@ataymano ataymano ataymano approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[RLOS2021]Make the library a proper python package
4 participants
@JuiP @jackgerrits @ataymano @cheng-tan