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 mechanisms for retrieving tags by position within a sequence #749

Merged
merged 5 commits into from Feb 8, 2015

Conversation

ctb
Copy link
Member

@ctb ctb commented Feb 1, 2015

This is primitive functionality enabling certain sorts of queries into the graph from within Python. /cc @camillescott; at some point we should chat more about how this intersects with labeling.

@ctb
Copy link
Member Author

ctb commented Feb 1, 2015

(a review would be welcome, but not yet ready for merge.)

@ctb
Copy link
Member Author

ctb commented Feb 7, 2015

  • Is it mergable?
  • Did it pass the tests?
  • If it introduces new functionality in scripts/ is it tested?
    Check for code coverage.
  • 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?
  • Was a spellchecker run on the source code and documentation after
    changes were made?

@ctb
Copy link
Member Author

ctb commented Feb 7, 2015

Ready for review @luizirber @camillescott @mr-c

return NULL;
}

if (strlen(kmer_s) < counting->ksize()) { // @@
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this comment?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

Py_BEGIN_ALLOW_THREADS

HashIntoType kmer, kmer_f, kmer_r;
kmer = _hash(kmer_s, counting->ksize(), kmer_f, kmer_r);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GCC complained about kmer being defined but not used in this function, do you see it being used in the future? If not maybe just ignore the return value and remove it from the previous line?

@ctb
Copy link
Member Author

ctb commented Feb 8, 2015

Thanks @luizirber - ready for re-review.

@luizirber
Copy link
Member

LGTM

luizirber added a commit that referenced this pull request Feb 8, 2015
Added mechanisms for retrieving tags by position within a sequence
@luizirber luizirber merged commit b63bd94 into master Feb 8, 2015
} catch (_khmer_signal &e) {
PyErr_SetString(PyExc_ValueError, e.get_message().c_str());
return NULL;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1356-1357 lack testing coverage and won't be called ever anyhow

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed!

On Tue, Feb 10, 2015 at 11:16:55AM -0800, Michael R. Crusoe wrote:

  • try {
  •    unsigned int pos = 1;
    
  •    KMerIterator kmers(seq, counting->ksize());
    
  •    while (!kmers.done()) {
    
  •        HashIntoType kmer = kmers.next();
    
  •        if (set_contains(counting->all_tags, kmer)) {
    
  •             posns.push_back(pos);
    
  •             tags.push_back(kmer);
    
  •        }
    
  •        pos++;
    
  •    }
    
  • } catch (_khmer_signal &e) {
  •    PyErr_SetString(PyExc_ValueError, e.get_message().c_str());
    
  •    return NULL;
    
  • }

1356-1357 lack testing coverage and won't be called ever anyhow


Reply to this email directly or view it on GitHub:

https://github.com/ged-lab/khmer/pull/749/files#r24441205

C. Titus Brown, ctbrown@ucdavis.edu

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks; making new pull request

@mr-c mr-c mentioned this pull request Feb 11, 2015
mr-c added a commit that referenced this pull request Feb 11, 2015
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