kiudee / cs-ranking Goto Github PK

Currently variables like learning_rate are specified as a tunable and it is not possible to continue training with a lower learning rate.

Thoughts?

It would be nice to check our documentation before committing / merging. Now that we're making use of the pre-commit framework, this should be easy. Options include

• https://github.com/PyCQA/doc8
• https://github.com/twolfson/restructuredtext-lint (though there's no native pre-commit config; here are two options to use it anyway)

We just need to make sure our docs pass the linter first. This issue might be relevant regarding warnings.

In csrank/discretechoice/nested_logit_model.py we make use of sequence indexing in theano multiple times. For example:

Here rows and cols are both 1d tensors which are then used to index a different tensor:

Theano complains that this is deprecated:

FutureWarning: Using a non-tuple sequence for multidimensional indexing is deprecated; use `arr[tuple(seq)]` instead of `arr[seq]`. In the future this will be interpreted as an array index, `arr[np.array(seq)]`, which will result either in an error or a different result.

I'm not sure how to fix this. According to the documentation, theano should support boolean mask indexing. So I thought we should be able to do

mask = tt.eq(self.y_nests, i)
utility = tt.set_subtensor(utility[mask], tt.dot(self.Xt[mask], weights[i]))

instead (as tt.eq should return a boolean mask). But unfortunately that doesn't work; it gives the same warning.

Any ideas?

There has been some internal discussion about this, but I think its time to also open an issue about it. We are still using tensorflow 1, which has been outdated for a while now. Switching to tensorflow 2 would be a significant effort, since the underlying model fundamentally changed (there is no explicit graph construction anymore). At that point, it may be worth evaluating switching to pytorch instead. pytorch is a newer, very popular autodiff framework.

This article comes to the conclusion that

TensorFlow is still mentioned in many more job listings that PyTorch, but the gap is closing. PyTorch has taken the lead in usage in research papers at top conferences and almost closed the gap in Google search results. TensorFlow remains three times more common in usage according to the most recent Stack Overflow Developer Survey.

Here's another relevant article. Overall it seems to me that pytorch is the more future-proof choice, and if we're going to have to rewrite a lot of the code anyway we might as well switch. I do not have any practical experience in pytorch yet though, that's just what I could determine from other's opinions and first impressions.

We should also think about how we want to do the transition. This is a major undertaking and probably will take a while. Should we support tf1 and newthing in parallel? Gradually move models to newthing (thereby having mixed support)? Fork the project? Work on one big PR/branch, effectively blocking most other work for the time due to potential conflicts?

Currently we import all submodules in __init__:

This results in the user seeing a confusing list of submodules. We should trim that by using __all__ to only import important classes and functions.

The remaining modules are still available, but hidden by default.

nDCG is expecting relevance scores as input. When supplying rankings, we first have to convert the ranking into a set of ordered scores. This can quickly lead to numerical problems, due to the exponential growth of 2 ** s.

Todo

• Implement a method of converting rankings to relevance stores, which ensures numerical stability for nDCG.
• Reactivate the test in test_metrics and account for the conversion.

During the tests, numpy complains about a "mean of empty slice". That happens because the calculation of the spearman correlation filters the labels it applies to as follows:

And then averages its results:

Which may be empty (or consist of only NaNs) due to the previous filter. What is the intention behind that filter?

CC @prithagupta

See #118 (comment) for details. Should be resolved after merging #118 to avoid conflicts.

Currently we only document the fit function.

We need the following notebooks:

• Usage of FATE-Network
• Usage of FETA-Network
• Run of experiments on synthetic data
• ...

Rationale

Most of the learners implemented in cs-ranking already implement an interface similar to the one described in https://scikit-learn.org/stable/developers/develop.html#rolling-your-own-estimator,
i.e., we usually have a fit and predict method implemented.
For users to be able to use all learners effortlessly in a scikit-learn pipeline.Pipeline or to apply model_selection.GridSearchCV, we should make sure that all additional requirements are also fulfilled.

To do

• Use get_params and set_params to set parameters. This is important, since GridSearchCV or BayesSearchCV call set_params for hyperparameter optimization. sklearn.base.BaseEstimator implements basic versions of these. The current way we handle hyperparameters should be deprecated.
• It is recommended to not do any parameter validation in __init__, but rather in fit itself. set_params is supposed to do exactly the same thing as __init__ with respect to parameters.
• Init parameters should be written without changes as attributes. All generated attributes should have a trailing _.
• There should be no mandatory parameters. The user should be able to run the learner without having to provide arguments.
• Implement a score method. This is helpful, since hyperparameter optimizers call this function by default. Otherwise the user has to implement a custom one.
• Implement clone methods for each learner.

Most of these changes are independent of each other and could be done using separate branches.

Currently, the scripts we use for our experiments are still part of this repository. Since this repository is moving towards being a library for object ranking and choice, this code should be moved to a separate repository.

We are using bump2version now to change the version number and create a tagged commit. This triggers an upload of the new version to PyPi.
At the same time the HISTORY.rst file needs to be updated with the recent changes.

This process should be documented in Sphinx.

The correct order is:

1. Update HISTORY.rst and commit.
2. Run bump2version [patch|minor|major]
3. Push commits and tag to master/branch.

We have a utility function configure_numpy_keras which is used in some of the experiment scripts:

It does the following:

• Set random seeds
• Sets the KERAS_BACKEND to Tensorflow
• Checks the number of GPUs and sets the Tensorflow options accordingly
• Creates a Tensorflow session for Keras to use

There are a few issues (and maybe more) with this:

• Everything is set to hardcoded constants. Making it configurable is desirable.
• log_device_placement is set to True, which can cause slowdowns due to logging and should be False by default.
• It is not clear, if tensorflow_util.py is the correct location, if the function is only ever used in experiments.
• It is not documented.

Rationale

scikit-optimize is currently not maintained anymore and BoTorch implements several features making it very useful for our library:

• Proper handling of hyper priors (including sensible defaults), which should help stabilize our runs
• Analytic and Monte-Carlo acquisition functions designed for noisy target functions
• Batching of hyperparameter runs (allowing parallel execution)

While working on fixing #126, I noticed that our online docs for choice functions are broken:

Notice that there are no details other than the name. The choice functions don't link to any additional documentation either. The docs for our other types of estimators work as expected.

The documentation at https://kiudee.github.io/cs-ranking/ returns a 404 error.
It appears our changes to travis-ci must have caused a problem and the documentation is not updated correctly.

Maintaining a running average of weights has been shown to improve generalization.
We should evaluate the effectiveness for our architecture(s).

Paper: Averaging Weights Leads to Wider Optima and Better Generalization

We have many warnings in the tests, since we accidentally escape characters in the docstrings.

Example:

Since the last release, we have improved the UX of our API, fixed unexpected behaviour, fixed a few bugs and did some refactoring. That's just off the top of my head, there are probably other things as well.

We should think about a new release.

Todo

• Update the README.rst
• Mirror the intro.rst
• Update API:
  • Check old API reference and fix if necessary
  • Include new settings
• Write example notebooks for both settings

See #129 (comment). We already declare many types in docstrings. Using mypy would require us to formalize this a bit more, with the added bonus of static guarantees and better tooling support (such as enhanced tab completion).

To prevent issues linke #137 we should have a CI check that tries to import csrank with only the minimal dependencies.

Rationale

When training the models on different datasets, it would be advantageous to be able to save the model as is to a file. That way it is easy to later load the model and e.g. evaluate it on new instances etc.

We have recently added some static analysis and formatting tools. One thing we are not checking for yet is inline documentation. There are some tools out there, for example pycodestyle. It seems like pylint has some doc checking functionality too.

I think it would be valuable to extend our static checks to the inline documentation. We already check stand-alone rst files with doc8..

The new Tunable class should be able to change the set of tunable parameters during runtime (currently it is a class method).
This would allow us to attach arbitrary numbers of parameters to a model (e.g. coming from callbacks etc).

• Change Tunable to be nestable
  • Methods should be object methods
• Change optimizer to use object methods

Potential problems

• Fit function is called after optimizer needs to know about parameters
• Is it realistic that a model can have parameters to be tuned which do depend on the model (and thus are not settable in advance by the user)?
• When the user provides tunable objects -> ensure that they reset properly across iterations.

Potential solutions

• Let optimizer handle an ordered dictionary of all the tunables.

The _predict_scores_fixed method of the class AllPositive requires X and Y inputs:

In the variadic case, it is called with X and Y:

However, it is called without Y in the predict_scores method for the non-variadic case:

This leads to a None prediction.

The fit method parameters are:

However the parameters are not passed to the super class:

The current testing setup is a source of frequent problems and should be simplified.

To do

• Switch to nox a "successor" to tox to manage the test environment setup
• Use pytest-xdist and/or parallel mode to parallelize the builds.
• (optional) Use tox-travis if we still want to use the build matrix on travis.

Related issues

#66, #97

Dataset generators like

Currently, the tests for the probabilistic models implemented in PyMC3 take a long time to run, which causes a long delay between updating a pull request and receiving Travis confirmation.

Measures

• Parallelize build using several environments
• Speed up PyMC3 tests
• Speed up installation process and optimize caching (if possible)

Currently, as soon as one of the parallel envs is finished, travis immediately tries to deploy to gh-pages and PyPI. This is obviously not desirable.
We should split the build into two stages 'build' and 'deploy' as described here:
https://docs.travis-ci.com/user/build-stages/matrix-expansion/

Rationale

For users it is much more convenient to be able to install the package from PyPI using a simple

pip install cs-ranking

or

conda install -c conda-forge cs-ranking

rather than checking out the repository.

What needs to be done

• Check the official packaging guidelines.
• Create a recipe for conda-forge.

Currently we require quite a few dependencies, which makes installing the library difficult. I went ahead and categorized the different dependencies (see below). We should do the following:

1. Remove these dependencies from install_requires
2. Move these to extras_require

Dependencies

• Core (mandatory):
  • numpy
  • scipy
  • scikit-learn
  • scikit-optimize
  • joblib
  • keras
  • tensorflow
  • docopt
• Data I/O or generation (optional):
  • psycopg2-binary for database access
  • pandas
  • h5py
  • pygmo
• Required for some of the probabilistic models (optional):
  • pymc3
  • theano
• Nice to haves (optional):
  • tqdm for progress bars

Simply running

import csrank

after installing csrank from pip without any optional dependencies, results in the following error:

csrank.util.MissingExtraError: Could not import the optional dependency theano. Please install it or specify the "probabilistic" extra when installing this package.

This should definitely not happen.

Version: 1.2.0

Currently, if the dataset generator receives an invalid dataset, it silently picks a default generator.

We should check all dataset generators/parsers for similar behavior.

A bug like #126 could have been cached by a static check for unused variables. We should think about using (at least some of) pylints checks.

• Bug in LRScheduler of callbacks
• Create LRSchedular and EarStopping independent of Keras implementation to avoid issues.
  One way would be to inherit from Callback from Keras.

There is an issue with the current LrScheduler. The problem is that it exponentially decreases the learning rate at each epoch after the epoch_drop.
For learning rate of 0.015 and epoch drop=5 and drop percentage =0.9

Epoch 00001: LearningRateScheduler setting learning rate to 0.014999999664723873.
Epoch 00005: LearningRateScheduler setting learning rate to 0.013499999698251487.
Epoch 00006: LearningRateScheduler setting learning rate to 0.012149999476969242.
Epoch 00007: LearningRateScheduler setting learning rate to 0.010934999864548446.
Epoch 00008: LearningRateScheduler setting learning rate to 0.009841500129550696
Epoch 00009: LearningRateScheduler setting learning rate to 0.00885734986513853.
Epoch 00010: LearningRateScheduler setting learning rate to 0.007174453390762211
Epoch 00011: LearningRateScheduler setting learning rate to 0.005811307118274272
While the output should be:
Epoch 00001: LearningRateScheduler setting learning rate to 0.014999999664723873.
Epoch 00005: LearningRateScheduler setting learning rate to 0.013499999698251487.
Epoch 00010: LearningRateScheduler setting learning rate to 0.012149999728426338.
Epoch 00015: LearningRateScheduler setting learning rate to 0.010934999755583704.

sklearn issues a warning during the tests:

sklearn.exceptions.UndefinedMetricWarning: F-score is ill-defined and being set to 0.0 in samples with no predicted labels.

This is because

• some of the test samples generated in csrank/tests/test_choice_functions.py:trivial_choice_problem have no true positives
• some of the learners predict no positives for some of the generated problems

In both of those cases the f-measure is not properly defined. sklearn assigns 0 and 1 respectively.

How should we deal with this? A metric should be defined for these possibilities. 0 and 1 in those cases seems somewhat reasonable, so maybe we should just silence the warning?

The current standards for specifying the build of a python project, its dependencies and the configuration of various tools is pyproject.toml. It may be a good idea to adopt this. There are some holdouts for the tools. Specifying the build system without needing to run a python program (which may have dependencies itself) is the biggest benefit.

It appears from the failed build 158 that the test

We should ensure, that it only fails if the underlying model is broken.

If the given regularization is not "l1" or "l2", "l2" is chosen, which seems undesirable as the parameter already has "l2" as the default value so None values do not need to be accounted for.

Now that we use a linter for formatting (#78), we could also use a semantic linter. Two common options are flake8, which is more conservative (less reports) and pylint. We would first need to address the issues those linters raise. For example for flake8 (ignoring line length, since black takes care of that and disagrees with flake8s limit):

$ flake8 **/*.py | grep -v 'line too long' | wc -l
36

Fixing those will likely improve the code anyway.

• Bug in predicting scores for dictionaries in feta discrete choice
• Refactor code for experiments

Rationale

Updating the version string when doing a new release is error-prone. Versioneer automatically determines the version string by querying git.
This allows us to simply tag a commit using a descriptive name like v1.1 or 1.1 and it will be applied automatically.

Being able to train the models from an instance generator would be very useful.

• Number of hidden layers and units
• Loss functions

• Learners
  • Object Ranking
  • Discrete Choice
  • General Choice Functions
• Dataset Parsers
• Metrics
• Losses