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

fix: exception is not imported sklearn.exceptions.NotFittedError #37

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

kman0
Copy link

@kman0 kman0 commented Feb 17, 2018

Currently the error is "NameError: name 'sklearn' is not defined" which is not the intended one as sklearn is not in scope. So adding a import statement.

currently the error is "NameError: name 'sklearn' is not defined"
1. Added option to save init parameters
2. n_features from core (more can be added)
@geffy
Copy link
Owner

geffy commented Feb 20, 2018

Hi! Thanks for contribution! Will check it carefully on weekends.

@geffy
Copy link
Owner

geffy commented Mar 4, 2018

Hi, @kman0! It seems like as-is this PR doesn't work.
This mini-example produces assertion error:

X_tr = np.random.randn(10000, 23)
y_tr = np.zeros(10000)
y_tr[::2] = 1

model = TFFMClassifier(
        order=2,
        rank=10,
        optimizer=tf.train.AdamOptimizer(learning_rate=0.001),
        n_epochs=50,
        batch_size=1024,
        init_std=0.001,
        reg=0.01,
        input_type='dense',
        seed=42
    )
model.fit(X_tr, y_tr, show_progress=True)
model.save_model('./tmp/model')
model.destroy()
del model

model = TFFMClassifier.load_model(TFFMClassifier, './tmp/model')

or it is supposed to be used in other way?

@kman0
Copy link
Author

kman0 commented Mar 18, 2018

@geffy my bad. I wanted to raise a pull req for the first commit alone! I was mainly working on Regression and didn't check it.

The assertion error occurs because of the hard setting that loss_function cannot be set in case of Classifier.

        assert 'loss_function' not in init_params, """Parameter 'loss_function' is
        not supported for TFFMClassifier. For custom loss function, extend the
        base class TFFMBaseModel."""

One way to overcome is to check if it is a TFFMClassifier instance and handle accordingly. I will post the changes shortly

@kman0
Copy link
Author

kman0 commented Mar 19, 2018

@geffy Your usage is correct. The primary reason why I wanted this was to train once and evaluate later.
If you merge this, you sample code could be included in example notebook.

One addition that I would make is a call to predict function in the example for the sake of completeness

model.predict(X_test)

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.

2 participants