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

Seed when computing hashes now a parameter for all C-level #79

Merged
merged 7 commits into from
Jan 4, 2017

Conversation

lgautier
Copy link
Contributor

@lgautier lgautier commented Jan 2, 2017

Initial changes to expose hashing seed used at C-level as a parameter (see issue #76).

If deemed reasonable this can be propagated up to command-line.

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

with Git (my own issue, not meaning Git had any... beside
making rather hairy to get out trouble).
@codecov-io
Copy link

codecov-io commented Jan 2, 2017

Current coverage is 76.47% (diff: 75.00%)

Merging #79 into master will increase coverage by 2.04%

@@             master        #79   diff @@
==========================================
  Files            15         17     +2   
  Lines          2010       2274   +264   
  Methods          46         48     +2   
  Messages          0          0          
  Branches         84         84          
==========================================
+ Hits           1496       1739   +243   
- Misses          489        510    +21   
  Partials         25         25          

Powered by Codecov. Last update 3138a76...487f8e3

@ctb
Copy link
Contributor

ctb commented Jan 4, 2017

Is this ready to be reviewed @lgautier ?

@ctb
Copy link
Contributor

ctb commented Jan 4, 2017

On a quick skim, the only problem I see is that the seed code isn't executed by any tests, nor is the seed saved in any signature/estimator location. I'm happy to address both of those post-merge over in #91, where I'm mucking about with that stuff.

@lgautier
Copy link
Contributor Author

lgautier commented Jan 4, 2017

It is in the sense that pull request is mostly consistent and self-sufficient. The only possible inconsistency is that until the seed is saved into JSON/YAML the consistency between save/load cycles can be broken, also this is not an issue for the casual user since the command line is not letting one play with the seed.

I am seeing the following missing:

  • bubbling the seed up to the command line interface
  • save the seed in the JSON / YAML (for the time it must be supported)
  • read the seed from the JSON (assume default seed with a warning if missing seed ?)

I'd say review and merge now, than quickly add the save/read seed to/from JSON/YAML ASAP.

@ctb
Copy link
Contributor

ctb commented Jan 4, 2017

Agreed! I can do the add/save etc in #91.

@ctb
Copy link
Contributor

ctb commented Jan 4, 2017

LGTM.

@ctb ctb merged commit 8561fda into sourmash-bio:master Jan 4, 2017
@lgautier
Copy link
Contributor Author

lgautier commented Jan 4, 2017

On a quick skim, the only problem I see is that the seed code isn't executed by any tests, nor is the seed saved in any signature/estimator location. I'm happy to address both of those post-merge over in #91, where I'm mucking about with that stuff.

Pickling / unplicking should not be problem since this seed is a default type... but I realizing while writing this that seed is not stored at the Python level. I am happy to give an hand in #91 about this if needed.

@lgautier lgautier mentioned this pull request Jan 4, 2017
3 tasks
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

3 participants