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

[MRG] remove __reduce__ from MinHash class #1144

Merged
merged 1 commit into from
Aug 4, 2020
Merged

[MRG] remove __reduce__ from MinHash class #1144

merged 1 commit into from
Aug 4, 2020

Conversation

ctb
Copy link
Contributor

@ctb ctb commented Aug 4, 2020

Removes __reduce__, which was needed for Python 2.7.

Fixes #895

NOTE: merge into latest, this is a breaking change for 3.x.

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

@ctb ctb added 4.0 issues to address for a 4.0 release breaking labels Aug 4, 2020
@ctb ctb changed the title remove __reduce__ from MinHash class [MRG] remove __reduce__ from MinHash class Aug 4, 2020
@codecov
Copy link

codecov bot commented Aug 4, 2020

Codecov Report

Merging #1144 into latest will increase coverage by 8.79%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           latest    #1144      +/-   ##
==========================================
+ Coverage   83.89%   92.68%   +8.79%     
==========================================
  Files          98       74      -24     
  Lines        9126     5810    -3316     
==========================================
- Hits         7656     5385    -2271     
+ Misses       1470      425    -1045     
Flag Coverage Δ
#rusttests ?

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

Impacted Files Coverage Δ
sourmash/_minhash.py 94.27% <ø> (-0.81%) ⬇️
sourmash/nodegraph.py 77.67% <0.00%> (-16.08%) ⬇️
src/core/src/index/search.rs
src/core/src/ffi/minhash.rs
src/core/src/errors.rs
src/core/src/wasm.rs
src/core/src/index/bigsi.rs
src/core/src/index/storage.rs
src/core/src/from.rs
... and 17 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 1f27f4f...df6324b. Read the comment docs.

@ctb ctb requested a review from luizirber August 4, 2020 18:24
Copy link
Member

@luizirber luizirber left a comment

Choose a reason for hiding this comment

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

I wonder if we should tag this commit as 4.0.0a0, to show how latest and stable diverged.

@ctb
Copy link
Contributor Author

ctb commented Aug 4, 2020

I wonder if we should tag this commit as 4.0.0a0, to show how latest and stable diverged.

I like that idea! Plus I think then it shows up in error messages etc.

I was really just trying out this PR as an (easily reversed) way to see how messy the whole process would be. So we can be experimental, too.

So, I'll take my time to merge and tag it, just to make sure it's all working. Thanks!

@ctb ctb merged commit cace054 into latest Aug 4, 2020
@ctb ctb deleted the remove_reduce branch August 4, 2020 20:44
@ctb
Copy link
Contributor Author

ctb commented Aug 4, 2020

@ctb
Copy link
Contributor Author

ctb commented Aug 4, 2020

argh! https://github.com/dib-lab/sourmash/releases/tag/v4.0.0a0 is correct. Deleted 4.0.0a0.

@ctb
Copy link
Contributor Author

ctb commented Aug 4, 2020

% sourmash info

== This is sourmash version 4.0.0a0. ==
== Please cite Brown and Irber (2016), doi:10.21105/joss.00027. ==

sourmash version 4.0.0a0
- loaded from path: /Users/t/dev/sourmash/sourmash/cli

@ctb ctb mentioned this pull request Aug 4, 2020
ctb added a commit that referenced this pull request Aug 5, 2020
* move sourmash._minhash to sourmash.minhash

* deprecate max_hash throughout

* change MinHash.add(...) to MinHash.add_kmer(...)

* remove update and is_molecule_type from MinHash

* remove subtract_mins

* rename downsample_n to downsample_num

* switch to hashes property instead of using get_mins()

* replace get_mins(...) with hashes thruout

* change deprecated 'compare' usage to 'similarity' in test_jaccard

* elminate most of the deprecation warnings in test__minhash by switching compare to similarity

* fix remaining tests in test__minhash

* fix compat message

* restore removed functions, sigh :)

* minor upd

* add deprecations

* use a wrapper object for .hashes and make it read-only

* refactor to use downsample(num/scaled=

* refactor to use downsample(scaled=...)

* return two deleted tests

* fixed test that was masked by another test

* add explicit check for length of kmer in add_kmer

* fix ordering in hash retrieval

* fix more tests for py2 <khaaaaaaaaaan>

* add 'flatten' method to MinHash

* add test for MinHash.flatten

* add tests for add and add_kmer

* remove nonsense test

* test the (now deprecated) get_mins function

* test (deprecated) get_hashes

* add tests for downsample and is_molecule_type

* test moltype properties more explicitly

* fix py27

* move translate_codon to module level

* put a stub in place of _minhash with a FutureWarning

* adjust import req

* remove __future__ imports

* remove sys.version checks for py 2

* remove requirement for enum34

* remove __reduce__ from MinHash class (#1144)

* avoid the DeprecationWarning

* update docs: only python 3.7 and 3.8

* remove 2.7 from travis

* remove _compat from signature.py

* remove _compat from exceptions.py

* remove _compat from index and sbt_storage

* remove _compat from nodegraph

* remove _compat completely

* make signature -> sig in CLI using py3 'aliases'

* put back assert that didn't work in py2

* Update sourmash/minhash.py

Co-authored-by: Luiz Irber <luizirber@users.noreply.github.com>

Co-authored-by: Luiz Irber <luizirber@users.noreply.github.com>
@ctb ctb mentioned this pull request Feb 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.0 issues to address for a 4.0 release breaking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

do we need __getstate__/__setstate__ *and* __reduce__ on MinHash objects?
2 participants