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

Add cold arm definition to MAB object #65

Merged
merged 15 commits into from
Feb 1, 2023
Merged

Add cold arm definition to MAB object #65

merged 15 commits into from
Feb 1, 2023

Conversation

dorukkilitcioglu
Copy link
Contributor

This PR adds cold arm set to the MAB object.

@dorukkilitcioglu
Add cold arm definition to MAB object
38729c8 
Signed-off-by: Kilitcioglu, Doruk <doruk.kilitcioglu@fmr.com>
@dorukkilitcioglu dorukkilitcioglu self-assigned this Jan 24, 2023
skadio
skadio previously approved these changes Jan 24, 2023
View reviewed changes
Copy link
Contributor

@skadio skadio left a comment

Choose a reason for hiding this comment

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

LGTM, one learning question and one design question

tests/test_mab.py Outdated Show resolved Hide resolved
mabwiser/_version.py Outdated Show resolved Hide resolved
@dorukkilitcioglu
Move cold arm and trained info to arm to status
091d07b 
Signed-off-by: Kilitcioglu, Doruk <doruk.kilitcioglu@fmr.com>
@dorukkilitcioglu
Update cold arm logic
93971bc 
Signed-off-by: Kilitcioglu, Doruk <doruk.kilitcioglu@fmr.com>
@dorukkilitcioglu
Bump version and remove unused code
b3adf53 
Signed-off-by: Kilitcioglu, Doruk <doruk.kilitcioglu@fmr.com>
bkleyn
bkleyn reviewed Jan 26, 2023
View reviewed changes
mabwiser/base_mab.py Outdated Show resolved Hide resolved
dorukkilitcioglu and others added 6 commits January 27, 2023 10:24
@dorukkilitcioglu
Update variable naming
b02a4e7 
Signed-off-by: Kilitcioglu, Doruk <doruk.kilitcioglu@fmr.com>
@bkleyn
no np warmstart
64351f3 
Signed-off-by: Kleynhans, Bernard <bernard.kleynhans@fmr.com>
@bkleyn
Merge branch 'cold_arms' of https://github.com/fidelity/mabwiser into…
d2e8c2a 
… cold_arms
@bkleyn
edits
1067b42 
Signed-off-by: Kleynhans, Bernard <bernard.kleynhans@fmr.com>
@bkleyn
drop status
1fb3cbc 
Signed-off-by: Kleynhans, Bernard <bernard.kleynhans@fmr.com>
@bkleyn
add tests
b667a28 
Signed-off-by: Kleynhans, Bernard <bernard.kleynhans@fmr.com>
@bkleyn bkleyn requested a review from skadio January 31, 2023 21:46
@bkleyn
Copy link
Contributor

bkleyn commented Jan 31, 2023

@skadio I wrapped up the changes from first round review

  • adding arm_to_status in BaseMAB that can be used to easily track if an arm is trained (arm was fit or partial fit), if an arm was warm started, and by what arm it was warm started
  • restricted warm start to Learning Policies as it is not clear how to interpret cold arms for Neighborhood Policies

skadio
skadio reviewed Jan 31, 2023
View reviewed changes
CHANGELOG.txt Outdated Show resolved Hide resolved
skadio
skadio reviewed Jan 31, 2023
View reviewed changes
mabwiser/base_mab.py Outdated Show resolved Hide resolved
skadio
skadio reviewed Jan 31, 2023
View reviewed changes
mabwiser/base_mab.py Outdated Show resolved Hide resolved
skadio
skadio reviewed Jan 31, 2023
View reviewed changes
mabwiser/base_mab.py Outdated Show resolved Hide resolved
skadio
skadio previously approved these changes Jan 31, 2023
View reviewed changes
Copy link
Contributor

@skadio skadio left a comment

Choose a reason for hiding this comment

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

LGTM. Minor comments

@bkleyn
edits
1495723 
Signed-off-by: Kleynhans, Bernard <bernard.kleynhans@fmr.com>
skadio
skadio previously approved these changes Feb 1, 2023
View reviewed changes
Copy link
Contributor

@skadio skadio left a comment

Choose a reason for hiding this comment

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

LGTM, and really like the idea of "Resolving Thread" as comments are fixed/taken into account. It cleans up the PR view and makes it easier to come back to the same PR for another pass (because now I only see "remaining" things) Good practice to follow in future and distill to the team

bkleyn added 2 commits February 1, 2023 10:06
@bkleyn
add comment
c9c1a0a 
Signed-off-by: Kleynhans, Bernard <bernard.kleynhans@fmr.com>
@bkleyn
add tests
6a5eed4 
Signed-off-by: Kleynhans, Bernard <bernard.kleynhans@fmr.com>
@bkleyn
Copy link
Contributor

bkleyn commented Feb 1, 2023

@skadio thank you for review! I addressed the final few comments and added some additional tests. I'm going to update the docs and merge.

bkleyn added 2 commits February 1, 2023 10:52
@bkleyn
update docs
b4beb68 
Signed-off-by: Kleynhans, Bernard <bernard.kleynhans@fmr.com>
@bkleyn
update docs
952970f 
Signed-off-by: Kleynhans, Bernard <bernard.kleynhans@fmr.com>
@bkleyn bkleyn self-requested a review February 1, 2023 16:27
@bkleyn bkleyn merged commit 881d0e9 into master Feb 1, 2023
@bkleyn bkleyn deleted the cold_arms branch February 1, 2023 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bkleyn bkleyn bkleyn approved these changes

@skadio skadio skadio left review comments

Assignees

@dorukkilitcioglu dorukkilitcioglu

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dorukkilitcioglu @bkleyn @skadio