-
Notifications
You must be signed in to change notification settings - Fork 13
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
Run bigscape v2 #251
Conversation
Seems like a few unrelated tests aren't passing? Let me know if I broke something. |
There was a problem hiding this 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.
There was a problem hiding this 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.
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. |
Quality Gate failedFailed conditions |
Thanks, the new commits look good to me. The unit tests are still missing for testing the |
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. |
I just killed the github actions to stop the unit tests. |
Oh no, they should already complete within two minutes... |
Okay it looks like this test just refuses to complete on the worker. I don't understand why; this is a very small sample. |
There was a problem hiding this 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.
There was a problem hiding this 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!
Co-authored-by: Cunliang Geng <[email protected]>
Quality Gate failedFailed conditions |
#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.