search

peterjc / pico_galaxy

Galaxy tools and wrappers for sequence analysis
17 stars 25 forks source link

Implement BED output #7

Closed hexylena closed 9 years ago

hexylena commented 9 years ago

I recently needed a BED output to do overlap detection with some downstream analysed features, and it was pretty easy to implement so I'm adding it here. It's non-optional, but then again you don't allow the user to enable/disable nuc/protein, so I'm just following existing convention.

peterjc commented 9 years ago

This is a nice surprise.

It looks good - having it as a non-optional output seems reasonable. I've not double checked the coordinates (0-based, 1-based, and forward vs reverse strand) but I guess you've already validated this with downstream usage?

I would want to add a test (just expand an existing test with the expected output).

Note to self: The Python script really does need a proper API at this point.

hexylena commented 9 years ago

Whoops, no I hadn't validated. Will do, and update with tests.

Agreed, this would be nice to have in biopython proper. I re-implemented that code at some point myself, not knowing that yours existed at the time.

peterjc commented 9 years ago

Fair point on pushing some of this into Biopython, probably under Bio.SeqUtils somewhere...

hexylena commented 9 years ago

(Ah, I'd assumed that was what you meant by proper API).

peterjc commented 9 years ago

No, I just meant using optparse for the command line API.

hexylena commented 9 years ago

Oh, I can add that to this PR if you'd like.

hexylena commented 9 years ago

Test cases added.

If you'd like it rewritten for optparse, I'm happy to do that since this script is saving me from writing another one-off local tool that would never make it into the toolshed.

peterjc commented 9 years ago

Well, if you're keen, that'd be most welcome.

I've done this already for many/most of the python scripts in this repository - e.g. https://github.com/peterjc/pico_galaxy/blob/master/tools/seq_filter_by_mapping/seq_filter_by_mapping.py - but if there's anything that could be done better I'd be curious for feedback.

peterjc commented 9 years ago

This will deserve a version bump. We'll also need to update the README.rst with the new feature (with an acknowledgement), and update the tar-ball instructions to include the *.bed files.

hexylena commented 9 years ago

@peterjc Done. Version bumped inside and out (v0.0.4 in tool, v0.0.8 in readme).

Edit: Forgot bed in tar-ball instructions, updating now

hexylena commented 9 years ago

Okay, BED files mentioned in (un)tar instructions.

peterjc commented 9 years ago

I'd bump everything to v0.1.0 given this is a fairly large set of changes.

hexylena commented 9 years ago

Done.

peterjc commented 9 years ago

Thank you Eric - pushed to the Test Tool Shed with some other unrelated tweaks like adding another translation table: https://testtoolshed.g2.bx.psu.edu/view/peterjc/get_orfs_or_cdss/65d76ca44cd2

Test Tool Shed tests were failing on this repository (which I think was a framework problem), hopefully this new revision will be tested in a day or two.

Do you need this on the main Tool Shed sooner?

hexylena commented 9 years ago

Nope, no rush.

man. 6. apr. 2015, 04.27 skrev Peter Cock notifications@github.com:

Thank you Eric - pushed to the Test Tool Shed with some other unrelated tweaks like adding another translation table: https://testtoolshed.g2.bx.psu.edu/view/peterjc/get_orfs_or_cdss/65d76ca44cd2

Test Tool Shed tests were failing on this repository (which I think was a framework problem), hopefully this new revision will be tested in a day or two.

Do you need this on the main Tool Shed sooner?

— Reply to this email directly or view it on GitHub https://github.com/peterjc/pico_galaxy/pull/7#issuecomment-89994604.