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

Release v0.1.9 #23

Merged
merged 26 commits into from
Feb 28, 2019
Merged

Release v0.1.9 #23

merged 26 commits into from
Feb 28, 2019

Conversation

BrikerMan
Copy link
Owner

add AVCNNModel, KMaxCNNModel, RCNNModel, AVRNNModel, DropoutBGRUModel, DropoutAVRNNModel model to classification task.

@alexwwang
Copy link
Collaborator

I reproduced the dim mismatch error during saved model loading.
I double checked the KMaxPooling layer in the referenced code file and compared it with the original one defined in VDCNN model. There are two tiny differences between these two editions and I suspect it's the reason arousing this shape mismatch error.
I would try to modify the layer function and this model tomorrow to prove it or not.

@alexwwang
Copy link
Collaborator

After one day investigation, I tend to suspect this would be caused by an unknown bug in Keras during loading a model with custom layer. As long as I alternate the KMaxPooling layer with factory GlobalMaxPooling1D layer, the error disappears.
But more works need to be done to find out the reason and solution.

At least two directions could be explored:

  • Complete the KMaxPooling layer define with more detailed class method. ( I've implemented compute_out_shape, build and get_config, but the error stays.)
  • Try to save and load the model in two steps, structure first and weights second. However I am not sure if this would work.

Some references: keras-team/keras#8612

@BrikerMan
Copy link
Owner Author

I have tried the model.model.load_weights('model/model.model'), it works just fine. But we have to refactor the save and load function.

@alexwwang
Copy link
Collaborator

I have tried the model.model.load_weights('model/model.model'), it works just fine. But we have to refactor the save and load function.

You tried it? It works?
I think it's a good news!
Then I would make a new PR first to update my modification to the layer and model implementation.
And then let's refactor the save/load process.

@BrikerMan
Copy link
Owner Author

I have tried this and this works fine, I think this means we could build the model first then load the saved weight later.

model = KMaxCNNModel()
model.fit(x, y, epochs=2)
model.save('model')

# works fine
model.model.load_weights('model/model.model')

# Error
KMaxCNNModel.load_model('model')

@alexwwang
Copy link
Collaborator

I have tried this and this works fine, I think this means we could build the model first then load the saved weight later.

model = KMaxCNNModel()
model.fit(x, y, epochs=2)
model.save('model')

# works fine
model.model.load_weights('model/model.model')

# Error
KMaxCNNModel.load_model('model')

Great!
I've refactored the s/l part and am testing the model again.
After its done, I'll make another PR.

@alexwwang
Copy link
Collaborator

I have tried this and this works fine, I think this means we could build the model first then load the saved weight later.

model = KMaxCNNModel()
model.fit(x, y, epochs=2)
model.save('model')

# works fine
model.model.load_weights('model/model.model')

# Error
KMaxCNNModel.load_model('model')

Your test may be not the exact situation.
I am afraid you should test a new model from scratch, say create it from the structure saved and then load weights to it.
This is what load_model is supposed to do but not load weights to an trained model.

@BrikerMan
Copy link
Owner Author

@alexwwang You are right, let me try a new model then load the weights to the new one.

@alexwwang
Copy link
Collaborator

@alexwwang You are right, let me try a new model then load the weights to the new one.

I tried, the problem is not gone. :(

Develop - bug fix and model improve
@BrikerMan
Copy link
Owner Author

Ok, let's release this version without the KMaxCNNModel model~

@alexwwang
Copy link
Collaborator

alexwwang commented Feb 27, 2019

Ok, let's release this version without the KMaxCNNModel model~

Well, but it's really annoying.

By the way, do you have any idea on supporting customized layer definition and registration to this scheme? Just like what you've done on customized embedding layers.
Otherwise each time someone creates a new model with certain customized layers, you have to refactor the model, splitting the layers define section out.

@alexwwang
Copy link
Collaborator

I've got an idea.
Since you have tested to load weights to a compiled model, I think it would worth a try to change the load process, rather than loading structure from json, but compile the model first and then load the weights.

@BrikerMan
Copy link
Owner Author

@alexwwang yes, maybe this will work.

@alexwwang
Copy link
Collaborator

alexwwang commented Feb 27, 2019

I've solved it!
Make a pr now.
Reference:
https://github.com/keras-team/keras/issues/5401#issuecomment-280100357

Alex Wang and others added 8 commits February 27, 2019 19:13
…pairs in hyper_parameters dict to be more flexible to config at model initialization
Develop - Fix shape mismatch bug in KMaxPooling, improve save load robust and config flexibility
add sequence length check at sequence labeling model
@BrikerMan BrikerMan merged commit 26f85b0 into master Feb 28, 2019
@BrikerMan BrikerMan changed the title v0.1.9 Release v0.1.9 Feb 28, 2019
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.

None yet

2 participants