Py 2.7/3.5+ and Keras 2/TF 1 compatibility needs to be tested.
cczhu opened this issue · 6 comments
Charles - Py 2.7/3.5
Ari - Keras & TF
Updated code successfully tested with python 3.6.3, Keras 2.0.8, TF 1.3.0. There were enough required changes to the model_train.py though that I think it makes sense to have a separate python script for it. Or maybe should there should still just be one python file and a variable that allows you to choose to build a keras v2 compatible model or keras v1 compatible model. What do you think @cczhu ? The downside of having two separate python scripts is that any future changes to non-model functions will have to be changed in two files vs. one...
Agreed that having two files that do almost the same thing is pretty clunky. I tried to streamline it into one file, and created some tests to go along with it. Please see pull request #11.
I did check that model.summary() looks the same in either Keras 1 or 2 using your code and my pull request, but didn't check if the entire script runs properly or not. Would you be able to check that? Also, it would be great if we could check something basic, like if you throw in a fake training set of ten 256x256 images compsed of all 1s or something and set the Keras seed to a fixed value, then you get exactly the same output in your code and mine. That kind of test could then go in the new test_model_train.py file.
Re: simple model test - I'm not sure if python guarantees that np.seed(42) is the same thing on your machine and mine. I know Hanno had to make internal functions for rebound to ensure that. Also, such a test might be testing more Keras stuff vs. DeepMoon stuff, i.e. input_data -> black_box -> output_data -> see if output_data matches on two machines. What if they don't match, what would that indicate? That something in Keras is broken? I guess that would be good to know.... I'm just not sure if there's anything we could do to fix it other than submit an issue with Keras. I'll think on that test.
Your compatibility solution looks really good! Thanks for taking the time to do that. Yes, I'll definitely check that your new streamlined code works and fix any minor bugs should they come up.
(This is a repeat of my e-mail to you; apologies for the duplication!)
I wasn't suggesting that we test Keras's internal functionality, but rather that we ensure that any future modifications we make doesn't mess with the CNN structure. Right now the tests I implemented just use the total number of trainable/non-trainable parameters as a kind of checksum, but ideally we'd be able to take all the information you get by doing model.get_config() and check things layer by layer. I fiddled with Keras a bit and unless we hack the backend its only possible to get the configuration printed to screen.
(Maybe I should submit a PR to Francois so that get_config has the option of printing to string, or to dict? :) )
If you can think of any additional tests to ensure that the model is doing predictable things, I think we should implement them. I agree that np.seed() is probably a bad idea, but what about keras.initializers.Ones() to set everything to 1?