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

DOC: add comparison to TF wrappers #133

Merged
merged 3 commits into from
Nov 27, 2020
Merged

Conversation

adriangb
Copy link
Owner

Closes #132

@codecov-io
Copy link

codecov-io commented Nov 25, 2020

Codecov Report

Merging #133 (8927856) into master (6555575) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #133   +/-   ##
=======================================
  Coverage   99.49%   99.49%           
=======================================
  Files           5        5           
  Lines         595      595           
=======================================
  Hits          592      592           
  Misses          3        3           

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 6555575...8927856. Read the comment docs.

README.md Outdated
@@ -6,6 +6,20 @@

Scikit-Learn compatible wrappers for Keras Models.

## Why SciKeras?

SciKeras is derived from and API compatible with `tf.keras.wrappers.scikit_learn`. The original TensorFlow (TF) wrappers are not actively mantained, and [may be deprecated](https://github.com/tensorflow/tensorflow/pull/37201#pullrequestreview-391650001) at some point. In addition, they have many incompatibilities with both the Keras ecosystem and the Scikit-Learn ecosystem. SciKeras attempts to resolve these issues by providing mantained, documented wrappers that are fully compatible with the entire Scikit-Learn and Keras ecosystems. Some advantages over the TF wrappers are:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some advantages over the TF wrappers are:

Is there documentation on how to go from TF wrappers to SciKeras wrappers?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Not currently. That would also be a good idea to add here. This stuff should also get added to the RTD docs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That would also be a good idea to add here

When I visted the RTD documentation, I expected to see the transition guide at the top of the "getting started" page.

This stuff should also get added to the RTD docs.

Yes, I agree. I tend to create READMEs that point directly to the HTML documentation. If I put anything on the README, it's very developer oriented (here's how to clone, directory structure, etc).

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think the "why SciKeras" section should be in both so that people who discover the package via GitHub aren't confused as to why it even exists before they open the docs.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I'm going to merge this as is (without any transition guide work). We have #124 to track the transition guide, and in particular, I would like to implement your suggestion in #124 (comment) since that will reduce the required changes. These changes would go beyond the scope of this PR.

@adriangb adriangb merged commit 94960da into master Nov 27, 2020
@adriangb adriangb deleted the docs/comparison-to-tf-wrappers branch November 27, 2020 22:52
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.

DOC: Add a doc page detailing advantages (and disadvantages?) vs. TF skelarn wrappers
3 participants