-
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
Adding feature: strain mapping locally #281
base: dev
Are you sure you want to change the base?
Conversation
@rtlortega - are the antismash results different due to the use of a different version? I.e., what version did you use for the latest results? |
@justinjjvanderhooft I haven't worked with antismash so far, so I can't tell. I didn't use antishmash results itself, I just re-arranged the naming of the folders as it should be, according to Dora and Annette's knowledge about it. I also know that the antismash folder results is incorrect because the stain_mappings in the test/data folder is wrong. |
@rtlortega well, it would be "wrong" if is not compatible with the version of antiSMASH used for the current code base of NPLinker, but it wouldn't be "wrong" if a newer version of antiSMASH has a different output than NPLinker expected. Anyways, in both case, it would be good to update the loading of the antiSMASH results to make it compatible.... |
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.
Hi Rosina, thanks for the PR. Before I review the details of the changes, let's first make sure all changes have followed required formats/styles as well as all tests have been added and passed. So, please fix the following issues first:
-
Please update your changes to make sure the code style and static typing are good using the tool
ruff
andmypy
. See related errors in this check: https://github.com/NPLinker/nplinker/actions/runs/11557614894/job/32277179283?pr=281 -
I did not see any new unit test in the folder
tests/unit
for your new functions/methods. Please add unit tests for your changes, and make sure they have passed before pushing them to the PR.
Regarding your comments:
I would strongly suggest that the test/data information is updated with the correct information as antismash results generates. The current strain_mapping is incorrect as well as the folder generated in test/unit/data/antismash. I generated another folder where the data is correct, but I was not sure if adding that to the pull request yet.
The data/files in the tests/unit/data
are independent with each other (as much as possible). So you don't have to care about the relationship between the antismash
data and the strain_mappings.json
, they are independent and are used for different unit tests.
You should add a new folder with new example data to test your new functions. Please remember: make the example data as simple/small as possible.
This is a new feature for generating the file needed for running NPLinker called strain_mappings in local mode.
The src and test were modified in:
-> antismash_loader.py -> creating a dict for genome --> bgcs
-> strain -> utils.py -> four new functions:
I also added the testing functions:
There is a notebook for running all the functions step by step: ~/nplinker/tests/unit/local_strain_mapping.ipynb
I would strongly suggest that the test/data information is updated with the correct information as antismash results generates. The current strain_mapping is incorrect as well as the folder generated in test/unit/data/antismash. I generated another folder where the data is correct, but I was not sure if adding that to the pull request yet.