datasnakes / OrthoEvolution

An easy to use and comprehensive python package which aids in the analysis and visualization of orthologous genes. 🐵
https://orthoevolution.readthedocs.io/en/master/
29 stars 4 forks source link

Rework the SGE module. #148

Closed grabear closed 4 years ago

grabear commented 5 years ago
What are you trying to accomplish?
What is the error or issue?

Just trying to make this module more "pythonic"

sdhutchins commented 5 years ago

If we're going to use snakemake can I remove the Luigi stuff from the Tools/SGE module??

Originally posted by @grabear in https://github.com/datasnakes/OrthoEvolution/issues/38#issuecomment-498750786

I decided to address that question here...

I think we could keep it. It's actually just a slight refactor (due to bugs) from luigi's SGE class that can be used for any qsub submission. I named it sgepipelinetask because I wasn't really sure what to name.

Honestly, it may be worth trying instead of the current sgejob.py module.

sdhutchins commented 5 years ago

I think we can definitely deprecate either the entire Pipeline module or the two python files within it. It may be worthwhile to keep the shell module just in case we need to write in some utility functions.

grabear commented 5 years ago

Ok. Since we are primarily working with PBSpro (vs SGE), I'm going to migrate the code to a pbs module. However, I'll keep the sge module for the sgepipelinetask.py script.

sdhutchins commented 5 years ago

Ok. Since we are primarily working with PBSpro (vs SGE), I'm going to migrate the code to a pbs module. However, I'll keep the sge module for the sgepipelinetask.py script.

That sounds good to me. They're similar or close enough in terms of actual functions that it can all be used for the same thing (we should not that at least).

grabear commented 5 years ago

I still need to do some work on #152, but I've gotten most of everything finished and I've tested the Qstat and Qsub classes out. They both work as far as I can tell. @sdhutchins I should be able to remove WIP from the pull request by tomorrow.

Things TODO:

grabear commented 5 years ago

Everything is done with the module except for updating the database_management stuff. I'm going to push that in a separate PR so it's not so confusing.

sdhutchins commented 5 years ago

@grabear Nice! That sounds good. I'll review this weekend - likely tomorrow.

image

grabear commented 5 years ago

I should have double-checked this, but I noticed some issues with PR #152.. @sdhutchins In some of the logic-based suggestions that you placed (and I accepted), there were some errors:

# This
if python_script is not None:

# Is changed to this
if not python_script:

# But that logic is not the same.  It's reversed.
# The proper logic change should look like this
if python script:

I'm re-opening this issue, to keep up with that. I'll make the changes in the next PR.

sdhutchins commented 5 years ago

@grabear Please do! I tried to recheck and make sure that I was doing the below instead of the above. Just an oversight or undersight? lol

# This
if python_script is None:

# Is changed to this
if not python_script: