DOC: Add history attribute to migration

[adriangb]

Closes #227

[codecov-io]

Codecov Report

Merging #228 (34b29a1) into master (f9f2092) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #228   +/-   ##
=======================================
  Coverage   99.71%   99.71%           
=======================================
  Files           6        6           
  Lines         693      693           
=======================================
  Hits          691      691           
  Misses          2        2           

---

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 f9f2092...34b29a1. Read the comment docs.

[github-actions]

📝 Docs preview for commit 34b29a1 at: https://www.adriangb.com/scikeras/refs/pull/228/merge/

[stsievert]

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_

[adriangb]

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

[stsievert]

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"]

[adriangb]

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!

[stsievert]

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.

[adriangb]

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.

[stsievert]

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_`."
        )

[adriangb]

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.

[stsievert]

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).

[adriangb]

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?