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

Document deciders.py #280

Merged
merged 9 commits into from
Oct 5, 2020

Conversation

david-z-shi
Copy link
Contributor

Reference issue

Closes #158 .

Type of change

Documents deciders.py

What does this implement/fix?

Wrote docstrings for the SimpleArgmaxAverage decider.

Additional information

@levinwil
Copy link
Collaborator

levinwil commented Oct 5, 2020

@latasianguy

  1. You should be PR’ing into the staging branch, not main
  2. Travis failed due to syntax errors

@david-z-shi david-z-shi changed the base branch from main to staging October 5, 2020 13:57
Copy link
Collaborator

@levinwil levinwil left a comment

Choose a reason for hiding this comment

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

Some small changes:

  1. On lines 26 and 31, please describe what classes actually is. You did it below as "A list of classes of type obj."
  2. On line 49, you say "Fits tree classification to transformed data X with labels y.". The data is NOT transformed here, it is input data. Also, this a decider, NOT a tree classifier.
  3. On line 102, ValueError is thrown when the labels have not been provided AND classes is empty
  4. On lines 140 and 192, you describe transformer_ids as "A list with all transformer ids". This is not true. It is a list of transformer_ids you would like to send the inference point through to be considered in the overall decision. Also, it technically does default to None, but you should say that it defaults to using ALL transformers.

Copy link
Collaborator

@levinwil levinwil left a comment

Choose a reason for hiding this comment

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

On line 52m you say "Fits the a Decider to the given voters and transformers."

That's grammatically incorrect. Also, it saves the transformers and voters internally. I'd just describe it as "Fits the decider to inputs X and final classification outputs y."

@levinwil levinwil merged commit d2300d2 into neurodata:staging Oct 5, 2020
@levinwil levinwil mentioned this pull request Oct 5, 2020
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.

Document deciders.py
2 participants