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

new method rename-chr for gff2gff.py #390

Merged

Conversation

kevinrue
Copy link
Contributor

@kevinrue kevinrue commented Apr 5, 2018

Equivalent to previous PR for gff2gff.

Please do not merge until unit tests are implemented.

@sebastian-luna-valero : I assume this is about adding more entries in https://github.com/CGATOxford/cgat/blob/master/tests/gff2gff.py/tests.yaml ?

@kevinrue
Copy link
Contributor Author

kevinrue commented Apr 5, 2018

Let's see if I guessed correctly, about those unit tests. Haven't played with this kind of YAML setup before.

@sebastian-luna-valero
Copy link
Member

Thanks!

Let's wait and see what travis says.

Did you also add the chip_peaks.bed file to the folder?

@sebastian-luna-valero
Copy link
Member

Sorry, just realised that the chip_peaks.bed is already there.

I think you need to use %DIR% for your ucsc2ensemble.txt input file to get the tests passing; i.e.:

options: --method=rename-chr --rename-chr-file=%DIR%/ucsc2ensembl.txt

rather than:

options: --method=rename-chr --rename-chr-file=ucsc2ensembl.txt

@kevinrue
Copy link
Contributor Author

kevinrue commented Apr 5, 2018

Oops. Didn't spot that one in the other examples. Funny how input/reference files don't need that. I can see why that would be, despite the confusion.

@kevinrue
Copy link
Contributor Author

kevinrue commented Apr 5, 2018

Tada!!
Thanks for the help @sebastian-luna-valero !

@sebastian-luna-valero
Copy link
Member

Great!

Before merging, could you please update this branch with the latest master? I have added in a couple of more tests, and I would like to see if this branch is ok with that.

Best regards,
Sebastian

@sebastian-luna-valero
Copy link
Member

Thanks, @kevinrue

You got pep8 errors. To reproduce the problem locally, just run: nosetests -v tests/test_style.py

You can then do: pep8 CGAT/scripts/gff2gff.py to find out where the error is.

Here is a table with the error codes explained:
https://pep8.readthedocs.io/en/1.4.6/intro.html#error-codes

Please let me know if you need further help.

Best regards,
Sebastian

@kevinrue
Copy link
Contributor Author

kevinrue commented Apr 6, 2018 via email

@sebastian-luna-valero
Copy link
Member

No worries, no rush.

Just wanted to make sure that the failed tests were not stopping you ;)

@kevinrue
Copy link
Contributor Author

kevinrue commented Apr 6, 2018

Did you disable some of the PEP8 checks for the CI?
I spotted and fixed the rules that my new lines broke, but pep8 also pointed out some existing lines that are >80 characters wide, e.g.:

@sebastian-luna-valero
Copy link
Member

Yes, some PEP checks are skipped:
https://github.com/CGATOxford/cgat/blob/master/tests/test_style.py#L32

That's why running nosetests -v tests/test_style.py should give you the same answer as the CI.

Best regards,
Sebastian

@kevinrue
Copy link
Contributor Author

kevinrue commented Apr 6, 2018

Seems to be ready to go 🎉

@sebastian-luna-valero
Copy link
Member

Thanks again!

@sebastian-luna-valero sebastian-luna-valero merged commit beffc47 into CGATOxford:master Apr 6, 2018
sebastian-luna-valero added a commit to cgat-developers/cgat-apps that referenced this pull request May 29, 2018
sebastian-luna-valero added a commit to cgat-developers/cgat-apps that referenced this pull request May 29, 2018
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