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

Run bigscape v2 #251

Merged
merged 57 commits into from
Jul 17, 2024
Merged

Run bigscape v2 #251

merged 57 commits into from
Jul 17, 2024

Conversation

adraismawur
Copy link
Collaborator

@adraismawur adraismawur commented Jun 3, 2024

#216
This implements running BiG-SCAPE 2.

I opted for keeping the run_bigscape function the same and changing how the arguments are built up to avoid a large amount of duplicated code. Let me know if a separate run_bigscape_v2 function is still desired.

The installation of BiG-SCAPE 2 will likely change in time. I have now fixed it to a reliable development version by commit, mirroring the BiG-SCAPE 1 installation. It's limited to low blob filesizes so should only minimally impact the installation time.

When we package BiG-SCAPE 2 we should be able to easily change this installation.

@adraismawur
Copy link
Collaborator Author

Seems like a few unrelated tests aren't passing? Let me know if I broke something.

@adraismawur adraismawur self-assigned this Jun 3, 2024
Copy link
Member

@CunliangGeng CunliangGeng left a comment

Choose a reason for hiding this comment

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

The changes look good to me. A few small things need to be updated. But please first solve the conflict to get the latest codebase before addressing my comments.

bin/install-nplinker-deps Show resolved Hide resolved
bin/install-nplinker-deps Outdated Show resolved Hide resolved
src/nplinker/nplinker_default.toml Outdated Show resolved Hide resolved
src/nplinker/nplinker_default.toml Outdated Show resolved Hide resolved
src/nplinker/nplinker_default.toml Outdated Show resolved Hide resolved
src/nplinker/genomics/bigscape/runbigscape.py Outdated Show resolved Hide resolved
Copy link
Member

@CunliangGeng CunliangGeng left a comment

Choose a reason for hiding this comment

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

There is not unit tests for the rubbigscape.py. It'd great to add them with some simple input data.

bin/install-nplinker-deps Show resolved Hide resolved
@adraismawur
Copy link
Collaborator Author

Sorry this took a while, getting the structure of BiG-SCAPE right to be a normal package was a bit of a hassle.

This should now cover all the issues.

Copy link

sonarcloud bot commented Jun 14, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@CunliangGeng
Copy link
Member

Thanks, the new commits look good to me.

The unit tests are still missing for testing the run_bigscape function, could you add that?

@adraismawur
Copy link
Collaborator Author

I've added some very rudimentary tests, they add --help as an argument so that the process returns successfully quickly.

I also tried adding some where the actual run is tested by inputting some files and checking for the existence of the expected output files, but I am seeing that it takes a relatively long time for v1 to finish, and for some reason v2 doesn't finish at all.

If wanted, I can still try adding this, but in theory the important code is covered with only these fast tests.

@CunliangGeng
Copy link
Member

I just killed the github actions to stop the unit tests.

@adraismawur
Copy link
Collaborator Author

Thanks for the update! Almost there.

I see the unit tests (github action) have been running for two hours ;-) Is it possible to have a lot more smaller antismash input (e.g. fake and mini gbk files) to make sure the running of bigscape complete within 2 mins? If it's not possible, I would prefer discarding these unit tests.

Oh no, they should already complete within two minutes...
I can check if it works with only two gbk files, or maybe I can make them very minimal by trimming the files themselves.
Otherwise I think we unfortunately need to remove those tests.

@adraismawur
Copy link
Collaborator Author

Okay it looks like this test just refuses to complete on the worker. I don't understand why; this is a very small sample.

Copy link
Member

@CunliangGeng CunliangGeng left a comment

Choose a reason for hiding this comment

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

OK, let's skip the test on CI, but keep running it on local computer, it's really fast.

A few more comments to address. After that, I think we are ready to release a new version of NPlinker.

nplinker.toml Outdated Show resolved Hide resolved
src/nplinker/genomics/bigscape/runbigscape.py Show resolved Hide resolved
src/nplinker/data/nplinker.toml Outdated Show resolved Hide resolved
tests/unit/genomics/test_runbigscape.py Outdated Show resolved Hide resolved
tests/unit/genomics/test_runbigscape.py Show resolved Hide resolved
Copy link
Member

@CunliangGeng CunliangGeng left a comment

Choose a reason for hiding this comment

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

Last two comments about docstring.

🎉 then you can directly merge the PR!

Copy link

sonarcloud bot commented Jul 17, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
73.2% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@adraismawur adraismawur merged commit f767bfa into dev Jul 17, 2024
5 of 6 checks passed
@adraismawur adraismawur deleted the run-bigscape-v2 branch July 17, 2024 14:09
This was referenced Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants