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

Add objective base classes to API reference #736

Merged
merged 12 commits into from May 1, 2020
Merged

Conversation

angela97lin
Copy link
Contributor

@angela97lin angela97lin commented Apr 29, 2020

@codecov
Copy link

codecov bot commented Apr 29, 2020

Codecov Report

Merging #736 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #736   +/-   ##
=======================================
  Coverage   99.29%   99.29%           
=======================================
  Files         140      140           
  Lines        4981     4981           
=======================================
  Hits         4946     4946           
  Misses         35       35           
Impacted Files Coverage Δ
...alml/objectives/binary_classification_objective.py 100.00% <ø> (ø)
.../objectives/multiclass_classification_objective.py 100.00% <ø> (ø)
evalml/objectives/regression_objective.py 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 61e5a4c...fd847b1. Read the comment docs.

@angela97lin angela97lin requested a review from dsherry Apr 29, 2020
:template: class.rst
:nosignatures:

ObjectiveBase
Copy link
Collaborator

@dsherry dsherry Apr 29, 2020

Choose a reason for hiding this comment

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

Looks like ObjectiveBase is missing the classmethod+property+abstractmethod fields: name, greater_is_better, score_needs_proba

@jeremyliweishih you were talking about this last week, right? In relation to the classproperty decorator (even though we don't use that in ObjectiveBase). Do we have an issue filed for that yet?

Copy link
Contributor

@jeremyliweishih jeremyliweishih Apr 29, 2020

Choose a reason for hiding this comment

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

Not exactly the same issue - to fix this problem it needs templating similar to whats in pipeline_class.rst and other templates with class properties. This will show the attribute and value but not the docstring. The issue with classproperty is that we can't access the __doc__ attribute which holds the doc strings. #652 is tracking that.

Copy link
Collaborator

@dsherry dsherry Apr 29, 2020

Choose a reason for hiding this comment

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

Ah, got it. Here's pipeline_class.rst. So we'd have to create an objective_class.rst which mirrors that but with the new field names?

Copy link
Contributor

@jeremyliweishih jeremyliweishih Apr 29, 2020

Choose a reason for hiding this comment

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

yeah! I was trying to find a more modular solution but I couldn't find one so the best we have now is to create an additional template.

Copy link
Contributor Author

@angela97lin angela97lin May 1, 2020

Choose a reason for hiding this comment

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

@jeremyliweishih Hmm, I'm trying some things out right now, but I can't get what we did for pipeline_class.rst to work in this case, since that shows the attribute and value but not the docstring... yet these functions only have docstrings?

ex of function:

    @classmethod
    @abstractmethod
    def name(cls):
        """Returns a name describing the objective."""

This is similar to component_graph on PipelineBase, which I noticed we also don't expose right now?

Copy link
Contributor

@jeremyliweishih jeremyliweishih May 1, 2020

Choose a reason for hiding this comment

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

oh yes you're correct! #652 tracks to fix that but I haven't found a good solution yet. Could you can add to that issue to show that we need to add for this as well? I’ll try to take a look when I can!

Copy link
Contributor Author

@angela97lin angela97lin May 1, 2020

Choose a reason for hiding this comment

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

Yah, will do! In this case @jeremyliweishih, @dsherry I'm going to address standardizing our language around problem_type, merge this PR, and file an issue to track the missing fields, and then link to #652 :)

Edit: filed #738 for this!

Copy link
Collaborator

@dsherry dsherry May 1, 2020

Choose a reason for hiding this comment

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

Ah dang, got it. Sounds good!

:nosignatures:

ObjectiveBase
BinaryClassificationObjective
Copy link
Collaborator

@dsherry dsherry Apr 29, 2020

Choose a reason for hiding this comment

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

BinaryClassificationObjective is missing can_optimize_threshold

ObjectiveBase
BinaryClassificationObjective
MulticlassClassificationObjective
RegressionObjective
Copy link
Collaborator

@dsherry dsherry Apr 29, 2020

Choose a reason for hiding this comment

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

I noticed binary/multi/reg each have different text in the docs for problem_type. Can we standardize that? I do like how in RegressionObjective we mention that its set to ProblemTypes.REGRESSION, so that's what I'd go with.

Copy link
Collaborator

@dsherry dsherry left a comment

@angela97lin thanks for doing this! This is great.

I left one comment about standardizing our language around problem_type.

Also, there are some fields missing from ObjectiveBase and BinaryClassificationObjective. I know @jeremyliweishih was dealing with a similar issue in the pipeline doc recently so maybe he has pointers. My suggestion would be to merge this PR and file an issue to track the missing fields.

Update: Jeremy posted his advice on how to add the missing fields, so if you decide to add that in this PR please ping me and I'll re-review!

@angela97lin angela97lin merged commit 185c540 into master May 1, 2020
2 checks passed
@dsherry dsherry deleted the 714_obj_api branch Oct 29, 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.

Missing API documentation for objective base classes
3 participants