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

update MinHash attributes/properties for consistency, and/or switch to using moltype attribute #1136

Open
ctb opened this issue Jul 31, 2020 · 6 comments

Comments

@ctb
Copy link
Contributor

ctb commented Jul 31, 2020

per this comment,

we have some fun inconsistencies in the molecule type in MinHash - we use MinHash.is_dna and MinHash.is_protein vs MinHash.hp and MinHash.dayhoff.

I see a few options moving forward -

  1. 'fix' the attributes so that it's is_hp and is_dayhoff;
  2. stop using those attributes and switch to moltype explicitly;
  3. use an enum for moltype instead.

this is basically a more specific proposal re #952 (enums for moltype) and ties into #751 (switching to explicit hash function incompatibility checking for MinHashes.)

@ctb
Copy link
Contributor Author

ctb commented Jul 31, 2020

just to be clear, I think we should skip over (1) and do (2) and (3), while deprecating .is_dna, .is_protein, dayhoff, and hp.

@luizirber
Copy link
Member

just to be clear, I think we should skip over (1) and do (2) and (3), while deprecating .is_dna, .is_protein, dayhoff, and hp.

I agree with this. .is_dna, .is_protein, dayhoff, and hp are mostly around to keep stuff working without breaking API, but since we are doing that for 4.0 anyway...

@ctb
Copy link
Contributor Author

ctb commented Aug 17, 2020

one minor counter-thought - it's not clear to me that enums provide a good UX experience for programmers. something I'll keep in mind as I dig in.

@luizirber
Copy link
Member

one minor counter-thought - it's not clear to me that enums provide a good UX experience for programmers. something I'll keep in mind as I dig in.

String-typed is initially more convenient in the constructor or return value for .moltype, but that doesn't last long. With enums you can:

  • use autocomplete with language servers in editors that support it
  • avoid the proliferation of .is_dna, .is_hp and other similar properties/methods for checking what is supported

What are the UX experience problems you're seeing?

@ctb
Copy link
Contributor Author

ctb commented Aug 17, 2020

the need to know which enums are there, to import them, and to supply them! I know, it's probably mostly just me. I'll see how it goes.

@ctb
Copy link
Contributor Author

ctb commented Mar 4, 2021

moltype property added in #936, is_molecule_type removed for 4.0 in #1128

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

No branches or pull requests

2 participants