Skip to content

Add LabelFractionPredictor with tests#332

Merged
latture merged 13 commits intomasterfrom
PLA-3759/label-fraction-predictor
May 12, 2020
Merged

Add LabelFractionPredictor with tests#332
latture merged 13 commits intomasterfrom
PLA-3759/label-fraction-predictor

Conversation

@genfx999
Copy link
Copy Markdown
Contributor

@genfx999 genfx999 commented May 8, 2020

Citrine Python PR

Description

https://citrine.atlassian.net/browse/PLA-3759

PR Type:

  • Breaking change (fix or feature that would cause existing functionality to change)
  • Bug fix (non-breaking change which fixes an issue)
  • [/] New feature (non-breaking change which adds functionality)

Adherence to team decisions

  • [/] I have added tests for 100% coverage
  • [/] I have written Numpy-style docstrings for every method and class.
  • I have communicated the downstream consequences of the PR to others.
  • I have bumped the version in setup.py

@genfx999 genfx999 requested a review from latture May 8, 2020 16:26
@latture
Copy link
Copy Markdown
Contributor

latture commented May 11, 2020

@genfx999 We're renaming this predictor IngredientFractionsPredictor and allowing multiple labels per predictor in mithril #529. Do you mind if I push the update to your branch?

@latture
Copy link
Copy Markdown
Contributor

latture commented May 11, 2020

Sorry for the last minute rename. I'll go ahead and push the changes. We can revert later if needed.

Overall, this PR was nearly perfect. The class and tests were great. There were only two minor things missing. We need to add the class to the map in Predictor.get_type, and we usually add all the exported predictors to the __all__ list at the top of predictors.py. I forgot to document the latter item. I added both changes to the latest commit. Thanks for picking up this card and getting the changes out so quickly.

@genfx999
Copy link
Copy Markdown
Contributor Author

@latture We good to merge this to formulations branch?

@latture
Copy link
Copy Markdown
Contributor

latture commented May 12, 2020

We just need to add user docs. I'll write those this morning.

@latture latture changed the base branch from feature/formulations to master May 12, 2020 20:31
@latture
Copy link
Copy Markdown
Contributor

latture commented May 12, 2020

@gregor-robinson @maxhutch Would one of you be able to do a quick review of this PR? I've reviewed it, and it looks good to me. However, I've pushed enough code to resolve merge conflicts with the feature branch it was based off of that a second pair of eyes would be good.

latture
latture previously approved these changes May 12, 2020
maxhutch
maxhutch previously approved these changes May 12, 2020
Copy link
Copy Markdown
Contributor

@maxhutch maxhutch left a comment

Choose a reason for hiding this comment

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

I made one non-blocking suggestion on documentation language. Looks good, though.

Comment thread src/citrine/informatics/predictors.py Outdated
Co-authored-by: Max Hutchinson <maxhutch@gmail.com>
@latture latture requested a review from maxhutch May 12, 2020 21:51
Copy link
Copy Markdown
Contributor

@maxhutch maxhutch left a comment

Choose a reason for hiding this comment

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

Re-stamp

@latture latture merged commit 8f6f40f into master May 12, 2020
@latture latture deleted the PLA-3759/label-fraction-predictor branch May 12, 2020 21:58
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.

3 participants