Skip to content

Rename counting_hash and hashbits at Python layer#1209

Merged
mr-c merged 12 commits into
masterfrom
fix/pynames
Aug 7, 2015
Merged

Rename counting_hash and hashbits at Python layer#1209
mr-c merged 12 commits into
masterfrom
fix/pynames

Conversation

@mr-c

@mr-c mr-c commented Jul 29, 2015

Copy link
Copy Markdown
Contributor

Fixes #872

@mr-c

mr-c commented Jul 29, 2015

Copy link
Copy Markdown
Contributor Author

Jenkins, retest this please

@ctb

ctb commented Jul 29, 2015

Copy link
Copy Markdown
Member

Also see #1032.

@mr-c

mr-c commented Jul 29, 2015

Copy link
Copy Markdown
Contributor Author
  • Is it mergeable?
  • Did it pass the tests?
  • If it introduces new functionality in scripts/ is it tested?
    Check for code coverage with make clean diff-cover
  • Is it well formatted? Look at make pep8, make diff_pylint_report,
    make cppcheck, and make doc output. Use make format and manual
    fixing as needed.
  • 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.
  • Is it documented in the ChangeLog?
    http://en.wikipedia.org/wiki/Changelog#Format
  • Was a spellchecker run on the source code and documentation after
    changes were made?
  • Is the Copyright year up to date?

@ctb

ctb commented Jul 29, 2015

Copy link
Copy Markdown
Member

What script/approach did you use for renaming?

@mr-c

mr-c commented Jul 29, 2015

Copy link
Copy Markdown
Contributor Author

@ctb I used

ls scripts/*.py tests/*.py sandbox/*.py khmer/*.py oxli/*.py | xargs sed -i 's/CountingHash/CountGraph/g'

Followed by git diff review and lots of testing.

@mr-c

mr-c commented Jul 29, 2015

Copy link
Copy Markdown
Contributor Author

Jenkins, retest this please

@ctb

ctb commented Jul 29, 2015 via email

Copy link
Copy Markdown
Member

@mr-c

mr-c commented Jul 29, 2015

Copy link
Copy Markdown
Contributor Author

@ctb @luizirber Ready for review & merge

@ctb

ctb commented Jul 30, 2015

Copy link
Copy Markdown
Member

My thoughts --

  • countinggraph is annoyingly long and graph would be an equally unambiguous choice in many sccripts - c.f. abundance-dist-single.py for an example. What do you think about just using graph?
  • do we want to change the print statements? e.g. see Saving k-mer counting table filename in filter-abund-single.py
  • there are a bunch of places in older scripts/ code where we're using htable; fix? It's not always just nodegraph, though.

See #1212 for some additional suggested changes.

@ctb ctb added this to the 2.0 milestone Aug 4, 2015
@mr-c mr-c mentioned this pull request Aug 6, 2015
Closed
@mr-c

mr-c commented Aug 6, 2015

Copy link
Copy Markdown
Contributor Author

Jenkins, retest this please

@mr-c mr-c closed this Aug 6, 2015
@mr-c mr-c reopened this Aug 6, 2015
@mr-c

mr-c commented Aug 6, 2015

Copy link
Copy Markdown
Contributor Author

Re:

  • countinggraph is annoyingly long and graph would be an equally unambiguous choice in many sccripts - c.f. abundance-dist-single.py for an example. What do you think about just using graph?

Did you mean countgraph? That would be find by me.

@mr-c

mr-c commented Aug 6, 2015

Copy link
Copy Markdown
Contributor Author

Does that mean all --savetables need to become --savegraph as well?

@ctb

ctb commented Aug 6, 2015

Copy link
Copy Markdown
Member

On Thu, Aug 06, 2015 at 03:09:43PM -0700, Michael R. Crusoe wrote:

Does that mean all --savetables need to become --savegraph as well?

They did when we switched away from savehash... so I guess so.

@ctb

ctb commented Aug 6, 2015

Copy link
Copy Markdown
Member

On Thu, Aug 06, 2015 at 02:57:13PM -0700, Michael R. Crusoe wrote:

Re:

  • countinggraph is annoyingly long and graph would be an equally unambiguous choice in many sccripts - c.f. abundance-dist-single.py for an example. What do you think about just using graph?

Did you mean countgraph? That would be find by me.

sure.

@mr-c

mr-c commented Aug 6, 2015

Copy link
Copy Markdown
Contributor Author

@ctb ready for review & merge

My apologies to everyone else who has to merge after this

@mr-c

mr-c commented Aug 6, 2015

Copy link
Copy Markdown
Contributor Author

Jenkins, retest this please

@ctb

ctb commented Aug 7, 2015

Copy link
Copy Markdown
Member

updating with master.

ctb added 2 commits August 7, 2015 10:52
@ctb
Merge branch 'master' of github.com:dib-lab/khmer into merge
a070b65 
Conflicts:
	ChangeLog
	khmer/khmer_args.py
	oxli/build_graph.py
	oxli/functions.py
	sandbox/optimal_args_hashbits.py
	scripts/normalize-by-median.py
	tests/test_scripts.py
@ctb
minor cleanup
5400837
@ctb

ctb commented Aug 7, 2015

Copy link
Copy Markdown
Member

@mr-c see 5400837 - if it looks good, please go ahead and merge. Thx!

@mr-c

mr-c commented Aug 7, 2015

Copy link
Copy Markdown
Contributor Author

Looks great, thanks!

mr-c added a commit that referenced this pull request Aug 7, 2015
@mr-c
Merge pull request #1209 from dib-lab/fix/pynames
44111f3 
Rename counting_hash and hashbits at Python layer
@mr-c mr-c merged commit 44111f3 into master Aug 7, 2015
@mr-c mr-c deleted the fix/pynames branch August 7, 2015 19:12
@mr-c mr-c mentioned this pull request Aug 7, 2015
Closed
@ctb ctb mentioned this pull request Aug 8, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2.0

Development

Successfully merging this pull request may close these issues.

2 participants

@mr-c @ctb