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

Removed preprocessing tests because they are already run in the "keras-preprocessing" repo. #10942

Closed
wants to merge 1 commit into from

Conversation

gabrieldemarmiesse
Copy link
Contributor

@gabrieldemarmiesse gabrieldemarmiesse commented Aug 19, 2018

Summary

I understood that it was one of the goals of the split of keras into three distinct repos. I don't know if it was one purpose that the tests were left here or if it was just that we forgot to remove them. I think this should be clarified.

Related Issues

#9256

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)

@fchollet
Copy link
Member

These tests are inexpensive and should be kept around in some form since these classes/functions are still part of the API. For reference, we still have applications tests, they're just a lot lighter than before (integration tests). In this case the existing unit tests are already very fast so there's limited upside in replacing them with brand new integration tests.

If you want to remove them, they would have to be replaced with thorough integration tests.

@fchollet fchollet closed this Aug 21, 2018
@gabrieldemarmiesse
Copy link
Contributor Author

Thank you very much for taking the time to clarify it.

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