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

Support Bigscape v2 data #234

Merged
merged 13 commits into from
Apr 4, 2024
Merged

Support Bigscape v2 data #234

merged 13 commits into from
Apr 4, 2024

Conversation

adraismawur
Copy link
Collaborator

This PR enables the user to load data from a BiG-SCAPE V2 database file. The data_sqlite.db file is expected to be in the same place as the mix_clustering_[cutoff].tsv would otherwise be.

If both files are found, the loader selects the tsv file.

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.

Thanks for adding the support of bigscape v2!

I left some minor comments for you.

One final comment on all files is that please remember to format all files using ruff. If you are also using vscode, the formatting could be automatic, see the guide.

src/nplinker/arranger.py Show resolved Hide resolved
src/nplinker/arranger.py Show resolved Hide resolved
src/nplinker/genomics/bigscape/bigscape_loader_v2.py Outdated Show resolved Hide resolved
src/nplinker/genomics/bigscape/bigscape_loader_v2.py Outdated Show resolved Hide resolved
src/nplinker/genomics/bigscape/bigscape_loader_v2.py Outdated Show resolved Hide resolved
src/nplinker/genomics/bigscape/bigscape_loader_v2.py Outdated Show resolved Hide resolved
src/nplinker/genomics/bigscape/bigscape_loader_v2.py Outdated Show resolved Hide resolved
src/nplinker/loader.py Show resolved Hide resolved
tests/unit/genomics/test_bigscape_loader_v2.py Outdated Show resolved Hide resolved
@CunliangGeng CunliangGeng mentioned this pull request Mar 27, 2024
2 tasks
@CunliangGeng
Copy link
Member

Just merged a PR to ignore docstrings for test files. So it's ok to not have docstrings for test functions.

I'm off from now, @gcroci2 could take over this PR.

@gcroci2
Copy link
Contributor

gcroci2 commented Mar 28, 2024

Just merged a PR to ignore docstrings for test files. So it's ok to not have docstrings for test functions.

I'm off from now, @gcroci2 could take over this PR.

Sure! @adraismawur just ask for my review again when you'll have integrated Cunliang's one :)

@adraismawur
Copy link
Collaborator Author

Alright! Changes were committed. Could you review this again?

src/nplinker/loader.py Outdated Show resolved Hide resolved
Copy link
Contributor

@gcroci2 gcroci2 left a comment

Choose a reason for hiding this comment

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

All good :) Thanks for the contribution!

The action for checking the format needs to pass before merging. Please run ruff as explained here and then commit the format/linting fixes.

@adraismawur
Copy link
Collaborator Author

Ok I have run the formatter, but there are files outside of those I edited in this PR that fail using ruff.

Should I rebase dev onto this branch and add formatting changes of other files to this one, or should I just merge as-is and the formatting can be done elsewhere?

@gcroci2
Copy link
Contributor

gcroci2 commented Apr 4, 2024

Ok I have run the formatter, but there are files outside of those I edited in this PR that fail using ruff.

Should I rebase dev onto this branch and add formatting changes of other files to this one, or should I just merge as-is and the formatting can be done elsewhere?

I still see:
src/nplinker/arranger.py:231:101: E501 Line too long (114 > 100)
Please make sure that the ruff checks pass on the files you edited (only) and then feel free to merge to dev. No worries about other files you didn't touch with this PR.

About the linting check failing, is this expected? @CunliangGeng If yes, I think we should exclude the checking on such files on the CI, otherwise it can be confusing for contributors.

@gcroci2 gcroci2 self-requested a review April 4, 2024 09:23
@adraismawur adraismawur merged commit 3a82fca into dev Apr 4, 2024
3 of 4 checks passed
@adraismawur adraismawur deleted the bigscape-v2 branch April 4, 2024 11:02
@CunliangGeng
Copy link
Member

CunliangGeng commented Apr 8, 2024

@gcroci2 @adraismawur

About the linting check failing, is this expected? @CunliangGeng If yes, I think we should exclude the checking on such files on the CI, otherwise it can be confusing for contributors.

Good point! I updated the github actions to only check changed files in PR #238.

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.

3 participants