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

Remove clear memory #150

Merged
merged 2 commits into from
Aug 21, 2020
Merged

Remove clear memory #150

merged 2 commits into from
Aug 21, 2020

Conversation

timokau
Copy link
Collaborator

@timokau timokau commented Aug 21, 2020

Description

Removing clear_memory and hash_file to reduce complexity and get one step further towards stateless / scikit-learn compatible estimators.

Motivation and Context

See description.

How Has This Been Tested?

Tests and pre-commit.

Does this close/impact existing issues?

Fixes #148, step towards #94 / #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.

@codecov
Copy link

codecov bot commented Aug 21, 2020

Codecov Report

Merging #150 into master will increase coverage by 0.47%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #150      +/-   ##
==========================================
+ Coverage   57.77%   58.25%   +0.47%     
==========================================
  Files         113      113              
  Lines        6960     6848     -112     
==========================================
- Hits         4021     3989      -32     
+ Misses       2939     2859      -80     
Impacted Files Coverage Δ
csrank/choicefunction/cmpnet_choice.py 94.59% <ø> (+2.28%) ⬆️
csrank/choicefunction/fate_choice.py 95.12% <ø> (+4.21%) ⬆️
csrank/choicefunction/feta_choice.py 66.20% <ø> (+0.66%) ⬆️
csrank/choicefunction/ranknet_choice.py 94.73% <ø> (+4.49%) ⬆️
csrank/core/cmpnet_core.py 97.72% <ø> (+10.34%) ⬆️
csrank/core/fate_network.py 70.15% <ø> (+3.49%) ⬆️
csrank/core/feta_network.py 64.89% <ø> (+3.43%) ⬆️
csrank/core/ranknet_core.py 97.72% <ø> (+10.34%) ⬆️
csrank/discretechoice/cmpnet_discrete_choice.py 100.00% <ø> (+3.22%) ⬆️
csrank/discretechoice/fate_discrete_choice.py 100.00% <ø> (+5.88%) ⬆️
... and 8 more

kiudee
kiudee previously approved these changes Aug 21, 2020
Copy link
Owner

@kiudee kiudee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thank you.

I agree with this simplification. Should it turn out that we need to clear memory in between different fit calls, we should anyway check what the best practices are.

@timokau
Copy link
Collaborator Author

timokau commented Aug 21, 2020

Travis has succeeded but that information apparently somehow got lost in transit. Can we restart that check?

@kiudee
Copy link
Owner

kiudee commented Aug 21, 2020

Restarting the build somehow did not fix it. I think the only thing we could do is to push something to trigger the whole process again. If that does not work I will investigate if there is some connection issue between Github and Travis CI (so far there is no error message).

Its use is no longer clear
(kiudee#148) and it introduces
unnecessary complications and additional state. Its removal also allows
us to remove the hash_file parameter.
Reduces complexity and brings us one step closer to removing state
(attributes) of estimators that is not set through the init parameters.
@kiudee
Copy link
Owner

kiudee commented Aug 21, 2020

Looks like this issue:
https://travis-ci.community/t/known-issue-travis-ci-reports-expected-waiting-for-status-to-be-reported-on-the-github-status-api-but-the-status-never-arrives/1154
I deactivated the status check for now until I update travis to the new format.

@timokau
Copy link
Collaborator Author

timokau commented Aug 21, 2020

Yeah I saw that too, very weird that it is just one report (all the previous ones seem outdated) and it isn't acknowledged yet.

Anyway, thanks for the review!

@timokau timokau merged commit 4e535f0 into kiudee:master Aug 21, 2020
@timokau timokau deleted the remove-clear-memory branch August 21, 2020 18:57
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.

clear_memory still needed
2 participants