autonomio / talos

Hyperparameter Experiments with TensorFlow and Keras
https://autonom.io
MIT License
1.62k stars 268 forks source link

Tidied up hidden_layers #336

Closed JanPokorny closed 5 years ago

JanPokorny commented 5 years ago

Hi, I have tidied up the function hidden_layers:

The test coverage should stay the same, as the functionality didn't change.

pep8speaks commented 5 years ago

Hello @JanPokorny! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! :beers:

Comment last updated at 2019-07-17 04:07:04 UTC
coveralls commented 5 years ago

Pull Request Test Coverage Report for Build 497


Changes Missing Coverage Covered Lines Changed/Added Lines %
talos/model/hidden_layers.py 7 8 87.5%
<!-- Total: 7 8 87.5% -->
Files with Coverage Reduction New Missed Lines %
talos/init.py 1 95.24%
talos/model/early_stopper.py 1 88.89%
talos/commands/deploy.py 2 92.45%
talos/scan/scan_addon.py 2 93.1%
<!-- Total: 6 -->
Totals Coverage Status
Change from base Build 500: 1.3%
Covered Lines: 1054
Relevant Lines: 1244

💛 - Coveralls
mikkokotila commented 5 years ago

Thanks but I think there is an issue.

For the common case where there is no kernel_initializer in params dictionary, the proposed:

kernel_regularizer=params.get('kernel_regularizer')

returns nothing. Whereas, the original:

    try:    
        kernel_initializer = params['kernel_initializer']   
    except KeyError:    
        kernel_initializer = 'glorot_uniform'

...returns glorot_uniform ... which is exactly what we want because it's the Keras default setting for the argument.

If there is a better way to do this (ideally have Talos automatically use Keras default setting if not set in the params dictionary), then that would be great.

JanPokorny commented 5 years ago

@mikkokotila Hello, you have confused kernel_initializer and kernel_regularizer! Take a second look, my code does precisely what the old did. Note that .get(...) returns the value of its second argument when the value is not present in the dictionary (or None if you don't specify it).

mikkokotila commented 5 years ago

Ah, got it, that's right I was confused indeed 👍

Ok I understand now, in that case this is great. Can you make the PR to daily-dev as that's the place for PR merges. It will make its way from there to dev and eventually master (and finally at some point to pypi i.e. production).

JanPokorny commented 5 years ago

@mikkokotila I was following CONTRIBUTING.md, you should update it then!

JanPokorny commented 5 years ago

@mikkokotila Also unsure why Travis is broken, it was broken even before I changed the destination branch. Looks like some problem with pickling. I think that this PR didn't cause it as it essentially doesn't change any behavior. Can you check that please?

mikkokotila commented 5 years ago

Sorry for taking ages with this. The big push to move from 0.5 to 0.6.3 took a couple of months in total, and this came a couple of weeks into that. Thanks again :)