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

provide .copy() method for both SourmashSignature() and MinHash #1485

Closed
ctb opened this issue Apr 25, 2021 · 6 comments · Fixed by #1551 or #1570
Closed

provide .copy() method for both SourmashSignature() and MinHash #1485

ctb opened this issue Apr 25, 2021 · 6 comments · Fixed by #1551 or #1570
Labels
good next issue An issue that should be ready to resolve.

Comments

@ctb
Copy link
Contributor

ctb commented Apr 25, 2021

I'm getting tired of using copy.copy(...) 😆

The idea here is to:

  • provide copy methods on both the SourmashSignature and MinHash classes. Naively, copy.copy(self) should work?
  • replace the calls to copy.copy(...) in the current sourmash code with the new method.
@ctb ctb added the good next issue An issue that should be ready to resolve. label May 8, 2021
@ctb
Copy link
Contributor Author

ctb commented May 8, 2021

note that MinHash in src/sourmash/minhash.py already contains a __copy__(self) method, could just add a copy = __copy__ below that. (And write some tests.)

@ctb
Copy link
Contributor Author

ctb commented May 9, 2021

actually, if we go with #1508, then we won't need .copy() for MinHash objects because you will instead just call mutable() or frozen() to get a copy for your desired purpose. Maybe we want to take a similar approach with SourmashSignature in the future?

@keyabarve
Copy link
Contributor

I would like to work on this. @ctb

@keyabarve
Copy link
Contributor

Could you provide a little clarification on what needs to be done here? Which files/functions need to be changed, etc.

@ctb
Copy link
Contributor Author

ctb commented May 24, 2021

Here's some reading, too - https://docs.python.org/3/library/copy.html,

In order for a class to define its own copy implementation, it can define special methods __copy__() and __deepcopy__(). The former is called to implement the shallow copy operation; no additional arguments are passed.

So:

  1. define __copy__(...) and copy(...) methods on MinHash subclasses in src/sourmash/minhash.py
  2. define __copy__(...) and copy(...) methods on SourmashSignature in src/sourmash/signature.py
  3. write tests to make sure they work properly, in tests/test_minhash.py and tests/test_signature.py respectively.

@keyabarve
Copy link
Contributor

Thanks, I will check it out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good next issue An issue that should be ready to resolve.
Projects
None yet
2 participants