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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MRG] Fix max hash deprecation warnings #1301

Merged
merged 7 commits into from
Feb 5, 2021
Merged

Conversation

ctb
Copy link
Contributor

@ctb ctb commented Feb 4, 2021

In #1128, we deprecated direct use of max_hash attribute as of 3.5, slated for removal in 5.0. This PR cleans up the sourmash codebase so that the associated deprecation warnings do not come from internal code, at least 馃槃

Specifically, this PR -

  • removes MinHash.max_hash from most code in sourmash, including tests.
  • creates a new private _max_hash proprety on class MinHash.
  • uses Minhash._max_hash in the few remaining places in the code base.

I also updated the screed version requirement in setup.cfg to v1.0.5, which removes another DeprecationWarning from old py2.7/3.6 code.

Fixes #1296, clean up deprecation warnings
Fixes #1297, update screed version

Checklist

  • Is it mergeable?
  • make test Did it pass the tests?
  • make coverage Is the new code covered?
  • Did it change the command-line interface? Only additions are allowed
    without a major version increment. Changing file formats also requires a
    major version number increment.
  • Was a spellchecker run on the source code and documentation after
    changes were made?

@codecov
Copy link

codecov bot commented Feb 4, 2021

Codecov Report

Merging #1301 (b821f76) into latest (3c1241b) will increase coverage by 5.26%.
The diff coverage is 93.93%.

Impacted file tree graph

@@            Coverage Diff             @@
##           latest    #1301      +/-   ##
==========================================
+ Coverage   88.70%   93.97%   +5.26%     
==========================================
  Files         125       98      -27     
  Lines       18293    14678    -3615     
  Branches     1438     1437       -1     
==========================================
- Hits        16226    13793    -2433     
+ Misses       1821      640    -1181     
+ Partials      246      245       -1     
Flag Coverage 螖
python 93.97% <93.93%> (+<0.01%) 猬嗭笍
rust ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage 螖
src/sourmash/compare.py 87.87% <酶> (酶)
src/sourmash/commands.py 82.39% <60.00%> (酶)
src/sourmash/minhash.py 94.54% <100.00%> (+0.36%) 猬嗭笍
tests/test__minhash.py 99.26% <100.00%> (酶)
tests/test_cmd_signature.py 100.00% <100.00%> (酶)
tests/test_jaccard.py 98.91% <100.00%> (+<0.01%) 猬嗭笍
tests/test_signature.py 94.97% <100.00%> (酶)
tests/test_sourmash_compute.py 100.00% <100.00%> (酶)
tests/test_sourmash_sketch.py 100.00% <100.00%> (酶)
src/core/src/ffi/signature.rs
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 3c1241b...b821f76. Read the comment docs.

@ctb
Copy link
Contributor Author

ctb commented Feb 4, 2021

ready for review @luizirber

@ctb ctb requested a review from luizirber February 4, 2021 15:06
@luizirber
Copy link
Member

Next PR down the line from this one is removing max_hash from MinHash.__init__, and I think that will remove most of the uses of MinHash._max_hash too?

@ctb
Copy link
Contributor Author

ctb commented Feb 5, 2021

Next PR down the line from this one is removing max_hash from MinHash.__init__, and I think that will remove most of the uses of MinHash._max_hash too?

Do you mean, that SHOULD be the next PR down the line? I'm not sure I want to do that for 4.0; involves a few more changes, and might break pickled MinHash objects.

@ctb ctb merged commit a49c997 into latest Feb 5, 2021
@ctb ctb deleted the fix_max_hash_deprecations branch February 5, 2021 00:21
@luizirber
Copy link
Member

Do you mean, that SHOULD be the next PR down the line? I'm not sure I want to do that for 4.0; involves a few more changes, and might break pickled MinHash objects.

I sure hope no one is depending on storing pickled MinHash objects 馃槵

@ctb
Copy link
Contributor Author

ctb commented Feb 5, 2021 via email

@luizirber
Copy link
Member

I sure hope no one is depending on storing pickled MinHash objects

I think I might be, in spacegraphcats

May be worth a look (faster and safer pickle): https://jcristharif.com/quickle/

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.

update required screed version to 1.0.5 resolve sourmash deprecation warnings for 4.0 release
2 participants