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

DOC: Add history attribute to migration #228

Merged
merged 8 commits into from
Apr 10, 2021
Merged

DOC: Add history attribute to migration #228

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
31177ab
add note on the history attribute
adriangb Apr 9, 2021
285c733
Single liner
adriangb Apr 9, 2021
4d6090e
Merge branch 'master' into add-history-to-migration
adriangb Apr 9, 2021
aa42bdd
fix rst
adriangb Apr 9, 2021
f1aafcc
Merge branch 'add-history-to-migration' of https://github.com/adriang…
adriangb Apr 9, 2021
be2507d
Update migration.rst
adriangb Apr 9, 2021
6f241e2
Update migration.rst
adriangb Apr 9, 2021
34b29a1
Update migration.rst
adriangb Apr 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions docs/source/migration.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,31 @@ SciKeras is largely backwards compatible with the existing wrappers. For most ca

SciKeras does however have some backward incompatible changes:

Fit returns ``self``
^^^^^^^^^^^^^^^^^^

In TensorFlow calling ``fit`` on wrappers returned a Keras ``History`` object, which held an attribute called ``history`` that is
a dictionary with loss and metric names as keys and lists of recorded values for each epoch as the dictionary values.
However, in SciKeras, ``fit`` now returns and instance of the estimator itself in order to conform to the Scikit-Learn API.
Instead, the history is saved in the ``history_`` attribute.
Calling ``fit`` resets this attribute, calling ``partial_fit`` on the other hand extends it.
Copy link
Collaborator

@stsievert stsievert Apr 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Would this edit be a little clearer?

In TF, calling Keras.fit returned a history object. However, in SciKeras, calling ``est.fitreturns the estimator to conform to the Scikit-learn API. The Keras history object is available through thehistory_` attribute of the estimator:

def get_model():
    ...
    return model

- model = get_model()
- hist = keras.fit(model, ...)
+ clf = KerasClassifier(model=get_model, ...)
+ hist = clf.fit(...).history_

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah that does seem a bit nicer, I'll incorporate it. Thank you.

Copy link
Collaborator

@stsievert stsievert Apr 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This example isn't 100% correct; see #227 (comment). Maybe the diff should be this?

- model = get_model()
- hist = keras.fit(model, ...).history
+ clf = KerasClassifier(model=get_model, ...)
+ hist = clf.fit(...).history_
losses = hist["loss"]

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yep you're right! The nuance of Keras returning a History object (which is a callback) that then contains the history attribute (which is a dict) was lost on me. Thank you for tracking it down and pointing it out!

Copy link
Collaborator

@stsievert stsievert Apr 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think this warrants adding a history attribute to the wrapper. I think a warning should be added that history is an unstable attribute that can be deleted without warning. I think this is warranted because it explicitly violates a core piece of the Scikit-learn API.

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 have three main qualms with it:

  1. In principle, I do not like replacing a complex object (tf.keras.callbacks.History) with another complex object (self/BaseWrapper) + a thin wrapper (DOC: Add history attribute to migration #228 (comment)) and expecting it to be backwards compatible.
  2. In practice, DOC: Add history attribute to migration #228 (comment) does not work if a user used to do clf.fit(...).set_params before since it would tf.keras.callbacks.History.set_params (this is documented method) but now would raise an AttributeError.
  3. I think it is okay to be backwards incompatible for this change, especially if we document it.

The tl;dr is that instead of trying to be somewhat backwards compatible, but probably not being fully backwards compatible, I feel that it is better to just document the change and move on.

Copy link
Collaborator

Choose a reason for hiding this comment

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

How 'bout this implementation?

class BaseWrapper:
    ...

    @property
    def history(self):
        raise AttributeError(
            "class 'BaseWrapper' has no attribute 'history'.\n\n"
            "Similar attributes include an attribute named `history_`."
        )

Copy link
Owner Author

Choose a reason for hiding this comment

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

Would you want to put that in permanently, or say for 1 minor version?

I still think that a breaking change is okay, especially across packages, so we don't necessarily need to add this, the documentation should be enough.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you want to put that in permanently, or say for 1 minor version?

Doesn't matter. The same error type is raised, and the first sentence of the message is the same.

I still think that a breaking change is okay, especially across packages, so we don't necessarily need to add this

Agreed. Documentation should be enough, but often is not enough (RTFD).

Copy link
Owner Author

Choose a reason for hiding this comment

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

Since the original issue was about documentation, and because I think the error is not strictly necessary, I would rather leave it out. Let's wait to see if the documentation is enough or if we get more issues before we add more code. If we do get more related issues, we can totally add something like that at a later date. Sound good?


.. code:: diff

def get_model():
...
model.compile(loss="mse", metrics=["mae"])
return model

clf = KerasClassifier(get_model)
- hist = clf.fit(...).history
- losses = hist["mae"]
+ hist = clf.fit(...).history_
+ losses = hist["mean_absolute_error"]

.. note::
Unlike the TensorFlow wrappers, SciKeras normalizes the names of the keys, so that if you use `metrics=["mae"]` you will get a key named `"mean_absolute_error"`.

One-hot encoding of targets for categorical crossentropy losses
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Expand Down