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

Do not copy params in get_params. #13598

Closed
wants to merge 1 commit into from
Closed

Conversation

mcarbajo
Copy link

@mcarbajo mcarbajo commented Dec 2, 2019

Summary

For compatibility with the scikit-learn clone function get_params has to return the same parameters as given in the init function. Fix issue #13586

Related Issues

#13586

PR Overview

  • This PR requires new unit tests [y/n] (make sure tests are included)
  • This PR requires to update the documentation [y/n] (make sure the docs are up-to-date)
  • This PR is backwards compatible [y/n]
  • This PR changes the current API [y/n] (all API changes need to be approved by fchollet)

For compatibility with the scikit-learn `clone` function `get_params` has to return the same parameters as given in the __init__ function. Fix issue keras-team#13586
@zachmayer
Copy link
Contributor

One test on python 3.6/TF 2.0 failed:
[gw1] [ 94%] FAILED tests/keras/utils/data_utils_test.py::test_ordered_enqueuer_timeout_threads

===== 1 failed, 1126 passed, 114 skipped, 49 warnings in 498.19s (0:08:18) =====
Using TensorFlow backend.
The command "if [[ "$MODE" == "INTEGRATION_TESTS" ]]; then PYTHONPATH=$PWD:$PYTHONPATH py.test tests/integration_tests; elif [[ "$MODE" == "PEP8_DOC" ]]; then PYTHONPATH=$PWD:$PYTHONPATH py.test --pep8 -m pep8 -n0 && py.test tests/docs; elif [[ "$MODE" == "API" ]]; then PYTHONPATH=$PWD:$PYTHONPATH pip install git+git://www.github.com/keras-team/keras.git && python update_api.py && pip install -e .[tests] --progress-bar off && py.test tests/test_api.py; elif [[ "$RUN_ONLY_BACKEND_TESTS" == "1" ]]; then PYTHONPATH=$PWD:$PYTHONPATH py.test  tests/keras/backend/; else PYTHONPATH=$PWD:$PYTHONPATH py.test tests/ --ignore=tests/integration_tests --ignore=tests/docs --ignore=tests/keras/legacy/layers_test.py --ignore=tests/test_api.py --cov-config .coveragerc --cov=keras tests/; fi" exited with 1.
cache.2
store build cache
Done. Your build exited with 1.

Maybe it's a flake and re-running will pass?

@mcarbajo
Copy link
Author

It could. Clearly the error is not related to the small change made

@zachmayer
Copy link
Contributor

@fchollet How do we re-run the tests on this PR? It seems like the test failure is a flake, but we are not sure.

@amueller
Copy link
Contributor

Any news on this?

@zachmayer
Copy link
Contributor

@amueller I think we just need to get someone from keras to re-run tests on this PR.

@fchollet Can you re-trigger travis on this PR, so we can get passing tests?

@zachmayer
Copy link
Contributor

This is a silly question, but how would I checkout this branch locally?

@cmarmo
Copy link

cmarmo commented May 12, 2020

Hi all, sorry for stepping in, I was investigating the scikit-learn issue 17022 and that led me here. I have checked out this branch: for me it fixes scikit-learn 17022 and scikit-learn issue 15722 opened by @mcarbajo himself.

This is a silly question, but how would I checkout this branch locally?

@zachmayer, here the command line procedure I used (I beg your pardon if I misunderstood your question)

$ git remote add mcarbajo https://github.com/mcarbajo/keras.git
$ git fetch mcarbajo
$ git checkout -b mcarbajopatch -t mcarbajo/patch-1 

Hope it could help to finalize this. Thanks for listening.

@zachmayer
Copy link
Contributor

@cmarmo thanks! I'm gonna try to make a new PR sometime and see if I can get this test to pass.

@johannesjung
Copy link

Any updates on this? @zachmayer, did you create a new PR?

@zachmayer
Copy link
Contributor

I still can't make my own PR:

Mayer-2:keras zach2$ git checkout -b mcarbajopatch -t mcarbajo/patch-1 
fatal: 'mcarbajo/patch-1' is not a commit and a branch 'mcarbajopatch' cannot be created from it

@cmarmo
Copy link

cmarmo commented Jul 13, 2020

@zachmayer do you mind checking that you have run git fetch mcarbajo first? Thanks.

@zachmayer
Copy link
Contributor

I did run git fetch mcarbajo first. I just tried again, same error. gh pr checkout 13598 worked for me though

@zachmayer
Copy link
Contributor

ok, the conflict in keras/wrappers/scikit_learn.py is gonna be hard to fix lol. The whole file is now just from tensorflow.keras.wrappers.scikit_learn import *

@fchollet Should we stop making PRs to the keras repo and try to fix this in the tensorflow repo?

@zachmayer
Copy link
Contributor

@zachmayer
Copy link
Contributor

@zachmayer
Copy link
Contributor

I made a PR to tensorflow here: tensorflow/tensorflow#41341

@fchollet fchollet closed this Dec 8, 2020
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

6 participants