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 override the optimizer's default parameters #142

Merged
merged 1 commit into from
Jun 11, 2020

Conversation

timokau
Copy link
Collaborator

@timokau timokau commented Jun 10, 2020

Description

As discussed in #119. The
reasoning is that we don't have good reason to override those, the
library user will likely have to tune them anyways (or use a different
optimizer altogether). At the same time, they make the design proposed
in #119 (passing uninitialized optimizers and their parameters
separately) more difficult.

Motivation and Context

#119 (comment)

How Has This Been Tested?

Tests & pre-commit hooks.

Does this close/impact existing issues?

Part of #119 and thus part of #116.

Types of changes

  • 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:

  • 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.

As discussed in kiudee#119. The
reasoning is that we don't have good reason to override those, the
library user will likely have to tune them anyways (or use a different
optimizer altogether). At the same time, they make the design proposed
in kiudee#119 (passing uninitialized optimizers and their parameters
separately) more difficult.
@codecov
Copy link

codecov bot commented Jun 10, 2020

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #142   +/-   ##
=======================================
  Coverage   59.96%   59.96%           
=======================================
  Files         116      116           
  Lines        7658     7658           
=======================================
  Hits         4592     4592           
  Misses       3066     3066           
Impacted Files Coverage Δ
csrank/choicefunction/cmpnet_choice.py 84.78% <ø> (ø)
csrank/choicefunction/fate_choice.py 91.30% <ø> (ø)
csrank/choicefunction/feta_choice.py 66.00% <ø> (ø)
csrank/choicefunction/ranknet_choice.py 81.63% <ø> (ø)
csrank/core/cmpnet_core.py 87.61% <ø> (ø)
csrank/core/fate_network.py 68.30% <ø> (ø)
csrank/core/feta_network.py 62.84% <ø> (ø)
csrank/core/ranknet_core.py 87.06% <ø> (ø)
csrank/discretechoice/cmpnet_discrete_choice.py 86.84% <ø> (ø)
csrank/discretechoice/fate_discrete_choice.py 94.44% <ø> (ø)
... and 7 more

@timokau timokau merged commit a1b36db into kiudee:master Jun 11, 2020
@timokau timokau deleted the keep-optimizer-defaults branch June 11, 2020 14:05
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.

2 participants