Properly pass on fit kwargs to the core network

[timokau]

Description

Previously we were passing on kwargs, but also specifying some arguments explicitly which meant they were not included in kwargs and not passed on.

To make this more maintainable, we now also delegate the documentation to the core.

Motivation and Context

126

How Has This Been Tested?

Does this close/impact existing issues?

Fixes #126

Types of changes

• [x] Bug fix (non-breaking change which fixes an issue)
• [ ] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• [x] My code follows the code style of this project.
• [ ] My change requires a change to the documentation.
• [ ] I have updated the documentation accordingly.
• [ ] I have added tests to cover my changes.

[timokau]

This fixes #126, but I'll check if there are more instances of this issue on our other estimators (I suspect there will be).

[timokau]

I have found no other obvious cases of this. Its very hard to screen for though. At some point we should add some more extensive static analysis, like pylint's "unused variable" which would have caught this.

Tests & pre-commit hooks pass. Test had to be adapted, since the test parameters (100 instead of 35 epochs, no validation split instead of 10%) are now actually respected.

While looking at the implementation and the test I also noticed that we are inconsistent in the typing of verbose. Sometimes we pass ints, sometimes booleans. We should think about introducing mypy typing.

Ready for review.

[timokau]

Thank you for the quick fix. We should think about, whether we want to make an intermediate release using master, or (which would be slightly more clean) branch off the last release and cherry pick all of the bug fixes for a bug fix release. Problem there is that we currently only release tags on master.

I think since cs-ranking is probably not deployed in too many production systems yet, we don't necessarily have to create a bugfix release that only contains the bugfix. If we decide to do that, we should still adhere to semver and simply make a full point release (1.2.0).

I think there have been no proper breaking changes since the last release right?

[timokau]

The alternative would be to branch off 1.1 at the point of the 1.1 tag, cherry-pick this commit and release 1.1.1. That would also be fine by me, just slightly more work.

[codecov[bot]]

Codecov Report

Merging #129 into master will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #129   +/-   ##
=======================================
  Coverage   60.01%   60.01%           
=======================================
  Files         116      116           
  Lines        7642     7642           
=======================================
  Hits         4586     4586           
  Misses       3056     3056           

Impacted Files	Coverage Δ
csrank/choicefunction/fate_choice.py	91.30% <ø> (ø)
csrank/core/fate_network.py	68.30% <ø> (ø)
csrank/tests/test_choice_functions.py	100.00% <ø> (ø)

[kiudee]

Yes, the problem is that currently we only deploy tags on master. We would need something like conditional deployment such that it will also work for branches.

In any case, the code seems stable enough to go to 1.2.0.

[timokau]

We probably need to look into this at some point anyway, but maybe not yet.