Migrate to PyTorch #191
Merged
Migrate to PyTorch #191
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is the start of the pytorch migration. Removing all tensorflow components as a first step simplifies the dependency situation.
They are no longer necessary since all tensorflow components were removed.
As suggested by pycodestyle E741.
A hotfix is available, but not committed to the repository. See the comment in `shell.nix` for more details.
They can also be useful for targets other than shell.nix, such as a nix-based pre-commit check.
Python 3.7 is not officially supported anymore. Python 3.9 is released already, but let's update to 3.8 first.
In preparation for the pytorch migration.
In preparation of adding new entries to the list.
This is part of the ongoing pytorch migration. We will use skorch as the basis for our pytorch estimators. That will make it easier to be compliant with the scikit-learn estimator API. Ranking and (general/discrete) choice estimators are often based on some sort of scoring. The task specific estimators make it easy to derive concrete estimators from a scoring module.
This adds a scoring module and the derived estimators for the FATE approach. The architecture is modular, so it should be easy to experiment with new ways to put the estimators together. This is a big commit. Multiple smaller ones that add the separate components (some of which are structural or can be useful outside of FATE and therefore could be considered features on their own) would probably have been better. Splitting it up now would take more time and is not worth it in this case though.
This simplifies interchangeable use of pytorch estimators and other estimators.
The "linear" implementations have been removed. The existing estimators do not expect `epochs` or `validation_split` parameters. The `verbose` parameter is accepted by some estimators, but defaults to `False` and is not expected by any of the ranking or discrete choice estimators.
The configuration is based on the configuration of the old (tensorflow based) fate estimators in the tests. The tensorflow tests used a 10% validation split, but still verified the performance in-sample. The validation was not actually used. Therefore I haven't kept that behavior. The performance isn't the same. Especially the performance on the choice task seems worse if we trust the test results. We shouldn't read too much into that yet. The test is mostly for basic functionality and not a reliable performance indicator. The sample size is small.
The binder logo (badge.svg vs badge_logo.svg) differs between the two files, but either should be good.
There is now a pytorch implementation of the FATE estimators.
This is similar to the "optimizer_common_args" dictionary that used to exist. This version contains skorch-specific arguments, which also includes the train split and the number of epochs. There are only the FATE based estimators now, but this would get repetitive when the other approaches are included again.
PyTorch migration: Remove tensorflow components, add FATE estimators
This is mostly an import from the "proof of concept" implementation with some small changes. It is just a starting point and not polished yet.
The old test of the tensorflow version only exercised the "no zeroth order model" configuration. This adds basic tests for both configurations.
These estimators (different tasks, same scoring module) are pretty repetitive. They are slightly different, so we need different classes. We could add an additional `FETABasedEstimator` mixin but that would probably just make it unnecessarily complicated. The current way to define the estimators also makes it easier to adjust them and play with different configurations. The repetitive definition and the additional mixin both have their advantages. I think we should stick to the repetitive definition for now.
The old test of the tensorflow version only exercised the estimator with the zeroth order model enabled. This adds basic tests for both configurations.
This is somewhat contrary to the comment I wrote in the general choice test. I have since noticed a similar issue with RankNet in the tests. It seems that SELU just works better, at least for these tiny toy problems. Its already the default for the other pytorch based estimators. I think it makes sense to use it as the default for CmpNet as well.
It can be useful to compare the network sizes that are used for the different estimators at a glance. The CmpNet test specifications already list the `n_hidden` and `n_unit` arguments even though they match the default values. This change makes the specifications more consistent.
I have added them in alphabetical order, which is why the order has changed in `discretechoice.rst`.
PyTorch migration: Finishing touches
For now we will only test Python 3.8 and run the following steps: * Pre-commit (incl caching) * Nox * Coverage
Migrate to GitHub Actions
Remove the build status, until GitHub Actions is running on master
kiudee
added
enhancement
Codecov Report
@@ Coverage Diff @@
## master #191 +/- ##
==========================================
- Coverage 57.04% 52.14% -4.91%
==========================================
Files 113 102 -11
Lines 6560 5090 -1470
==========================================
- Hits 3742 2654 -1088
+ Misses 2818 2436 -382
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Replaces Tensorflow v1.x with PyTorch 1.8.
The most important neural network learners have been converted:
In order to fully support scikit-learn interoperability, we also include skorch now as dependency.
The old master branch corresponding to v1.x.y will be continued here to facilitate bug fixes:
https://github.com/kiudee/cs-ranking/tree/v1.x
Motivation and Context
The code base depends heavily on Tensorflow v1, which is no longer updated due to the major update to Tensorflow v2. Since a major rewrite to support Tensorflow v2 would have been necessary, we deliberated whether it is better to switch over to PyTorch instead. Finally, we decided to go for PyTorch, since we also switched to it on other projects and the momentum of the research community is currently behind it.
Special thanks go to @timokau who worked hard on making this migration a reality.
How Has This Been Tested?
The tests of the learners have been adapted to use PyTorch. The tests were run locally and on GitHub Actions to confirm that everything passes.
Does this close/impact existing issues?
Resolves #125, resolves #153, resolves #146, fixes #130, closes #105, closes #43.
Types of changes
Checklist: