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

Added Seqhash functions #6

Merged
merged 13 commits into from
Jun 18, 2020
Merged

Added Seqhash functions #6

merged 13 commits into from
Jun 18, 2020

Conversation

Koeng101
Copy link
Contributor

This adds seqhash functionality to the Poly library. Basically, the Seqhash function hashes any DNA/RNA/Protein sequence with the blake3 algorithm to give it a universal identifier. If the sequence is circular, like a plasmid, it first takes the lexicographically minimal string rotation using Booth's algorithm, then hashes the resulting sequence with the blake3 hash.

@TimothyStiles
Copy link
Collaborator

TimothyStiles commented Jun 13, 2020

@Koeng101 this looks awesome! I want to refactor and expand on this before merging but to do that I need two things.

1.) Can you run go mod tidy && git add go.sum go.mod and push those files to the prime branch of your fork? It helps with dependency tracking stuff.

2.) Can you also give me push access to your fork so I can make those changes there then merge them into the prime branch of poly? This is the best way I know of to make sure github credits you for this pull request while still allowing me to edit it before being merged.

🙏 🙏 🙏

@Koeng101
Copy link
Contributor Author

1.) Run + added + committed the command above to the prime branch of the fork.

2.) Gave access to fork.

@Koeng101
Copy link
Contributor Author

Read comments, looks good. The reason for uppercasing is that the case never matters sequence-wise, but it impacts hashing. Could be lowercase for the same reason, but uppercase is nicer to read for sequences.

Also, the check failed because you didn't fix the hash_test.go file to use seqhash instead of Seqhash.

@TimothyStiles
Copy link
Collaborator

TimothyStiles commented Jun 16, 2020

So I just made a big push and a couple of smaller comment related commits. Everything is looking good so far but testing is still needed before this is merged.

Key things to test:

  • BoothLeastRotation needs to be tested such that it's fed every rotated permutation of a circular sequence then all outputs need to be equal to eachother.

  • Command usage. User experience. Make sure everything behaves as expected.

I don't see the need to test for individual hash functions. Seems tedious.

@TimothyStiles
Copy link
Collaborator

All basic commands have been implemented and tests passed. Please review before I merge @Koeng101.

Copy link
Contributor Author

@Koeng101 Koeng101 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

@Koeng101 Koeng101 merged commit b134e47 into bebop:prime Jun 18, 2020
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.

None yet

2 participants