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

Local dataset #208

Merged
merged 10 commits into from
Sep 16, 2022
Merged

Local dataset #208

merged 10 commits into from
Sep 16, 2022

Conversation

1511878618
Copy link
Contributor

Reference Issues/PRs

No related issues/PRs

What does this implement/fix? Explain your changes

support for loading local pdb files by ProteinGraphDataset and InMemoryProteinGraphDataset.

What testing did you do to verify the changes in this PR?

python -m py.test tests/, a part result of testing is as below:
image

Pull Request Checklist

  • Added a note about the modification or contribution to the ./CHANGELOG.md file (if applicable)
  • Added appropriate unit test functions in the ./graphein/tests/* directories (if applicable)
  • Modify documentation in the corresponding Jupyter Notebook under ./notebooks/ (if applicable)
  • Ran python -m py.test tests/ and make sure that all unit tests pass (for small modifications, it might be sufficient to only run the specific test file, e.g., python -m py.test tests/protein/test_graphs.py)
  • Checked for style issues by running black . and isort .

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@a-r-j
Copy link
Owner

a-r-j commented Sep 14, 2022

Thanks @1511878618 will give this a review.

@rg314 Paging for a consult on the failing docker build. It seems

RUN jupyter nbextension enable --py widgetsnbextension is throwing an error:

Jupyter command `jupyter-nbextension` not found.
The command '/bin/sh -c jupyter nbextension enable --py widgetsnbextension' returned a non-zero code: 1
Service 'graphein-cpu' failed to build : Build failed

Any ideas?

Copy link
Owner

@a-r-j a-r-j left a comment

Choose a reason for hiding this comment

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

LGTM, happy to merge if we can resolve this docker issue - I think it's unrelated and probably caused by some version drift.

Thanks again for a great PR!

@1511878618
Copy link
Contributor Author

1511878618 commented Sep 14, 2022 via email

@1511878618 1511878618 closed this Sep 14, 2022
@rg314
Copy link
Contributor

rg314 commented Sep 14, 2022

jupyter command since i could use ssh or vscode to connect the container. The reason of the bug, i guess, is that no commands in Dockerfile would install jupyter-nbextension. And in my view, it is not necessary quesstion since it is a support for extension of jupyter notebook  while jupyter lab

Hey @1511878618,

I agree that it's unnecessary however I'd kept it in so the example notebooks could be run in a containerized environment out of the box. The reason being it make it really easy for new users to quickly spin up a container and have the code running. As you say jupyter lab might be a better choice but then you'd need to test that the notebooks run in the containerized env. I would just fix the line rather than commenting it out.

I'll leave this up to @a-r-j

Let me know if you have any other questions.

@1511878618
Copy link
Contributor Author

1511878618 commented Sep 14, 2022

Thanks @1511878618 will give this a review.

@rg314 Paging for a consult on the failing docker build. It seems

RUN jupyter nbextension enable --py widgetsnbextension is throwing an error:

Jupyter command `jupyter-nbextension` not found.
The command '/bin/sh -c jupyter nbextension enable --py widgetsnbextension' returned a non-zero code: 1
Service 'graphein-cpu' failed to build : Build failed

Any ideas?

I solve this like below:
image

And woooooops, i closed this comment. 🤣

I reopen it, and I commited the changes of Dockerfile, if you are interested it.

@1511878618 1511878618 reopened this Sep 14, 2022
@a-r-j
Copy link
Owner

a-r-j commented Sep 14, 2022

Thanks @rg314

I'm inclined to leave it in (and it looks like @1511878618 has figured it out!).

I'm just a little confused as to why this issue emerged now; perhaps it's been unbundled from some other dependency? This hasn't been a problem before and we've not touched dependencies/the dockerfile for quite some time.

@1511878618
Copy link
Contributor Author

Thanks @rg314

I'm inclined to leave it in (and it looks like @1511878618 has figured it out!).

I'm just a little confused as to why this issue emerged now; perhaps it's been unbundled from some other dependency? This hasn't been a problem before and we've not touched dependencies/the dockerfile for quite some time.

Maybe the version is not specified for these packages like below shows:
image

or just unfortunately, these package update and no longer need for nbextension 🤣
How about we do this in docker container (and maybe we could put the container or minor into docker hub, so just docker pull ), and so our environment is fixed,

@a-r-j
Copy link
Owner

a-r-j commented Sep 14, 2022

How about we do this in docker container (and maybe we could put the container or minor into docker hub, so just docker pull ), and so our environment is fixed

I'm not much of a docker user but this sounds smart. If you and @rg314 agree on this then I'm happy to do that

@rg314
Copy link
Contributor

rg314 commented Sep 14, 2022

Thanks @rg314
I'm inclined to leave it in (and it looks like @1511878618 has figured it out!).
I'm just a little confused as to why this issue emerged now; perhaps it's been unbundled from some other dependency? This hasn't been a problem before and we've not touched dependencies/the dockerfile for quite some time.

Maybe the version is not specified for these packages like below shows: image

or just unfortunately, these package update and no longer need for nbextension 🤣
How about we do this in docker container (and maybe we could put the container or minor into docker hub, so just docker pull ), and so our environment is fixed,

Off the top of my head I don't have a quick fix but I'm sure someone else has run into the same problem with the notebook. If you can fix by specifying the old version this would be good.

From which line are you suggesting to move to Docker hub? It maybe that in the future the version of pytorch needs to be bumped with means someone would need to manage that image on docker hub (not ideal).

If you can fix by setting the version that I can have a look in 3 hours.

@1511878618
Copy link
Contributor Author

Thanks @rg314
I'm inclined to leave it in (and it looks like @1511878618 has figured it out!).
I'm just a little confused as to why this issue emerged now; perhaps it's been unbundled from some other dependency? This hasn't been a problem before and we've not touched dependencies/the dockerfile for quite some time.

Maybe the version is not specified for these packages like below shows: image

or just unfortunately, these package update and no longer need for nbextension 🤣
How about we do this in docker container (and maybe we could put the container or minor into docker hub, so just docker pull ), and so our environment is fixed,

Off the top of my head I don't have a quick fix but I'm sure someone else has run into the same problem with the notebook. If you can fix by specifying the old version this would be good.

From which line are you suggesting to move to Docker hub? It maybe that in the future the version of pytorch needs to be bumped with means someone would need to manage that image on docker hub (not ideal).

If you can fix by setting the version that I can have a look in 3 hours.

Ok, i'm not sure whether i can find the specific version of each package. Hmmmm, i think the best choice in my mind is add pip install jupyter_contrib_nbextensions and maybe conda could do it, too.

Actually, i have no idea about how to manage the version of packages which are needed in our project and how to make sure it still works with latest version of them.

@a-r-j
Copy link
Owner

a-r-j commented Sep 14, 2022

Looks like it's working. Would you be able to fix the failing dataloader test @1511878618 ?

@1511878618
Copy link
Contributor Author

Looks like it's working. Would you be able to fix the failing dataloader test @1511878618 ?

Looks like it's working. Would you be able to fix the failing dataloader test @1511878618 ?

Ok, I'll try later, as i'm not familiar with pytest, so maybe a long time 🤣

@rg314
Copy link
Contributor

rg314 commented Sep 15, 2022

Looks like it's working. Would you be able to fix the failing dataloader test @1511878618 ?

Looks like it's working. Would you be able to fix the failing dataloader test @1511878618 ?

Ok, I'll try later, as i'm not familiar with pytest, so maybe a long time 🤣

Fixed in 1511878618#1 @1511878618. @a-r-j I also had to fix some bits with the SSL. I think there's something wrong with the SSL certs on regnetworkweb.org.

Fixed Dockerfile and unit tests
@1511878618
Copy link
Contributor Author

Looks like it's working. Would you be able to fix the failing dataloader test @1511878618 ?

Looks like it's working. Would you be able to fix the failing dataloader test @1511878618 ?

Ok, I'll try later, as i'm not familiar with pytest, so maybe a long time 🤣

Fixed in 1511878618#1 @1511878618. @a-r-j I also had to fix some bits with the SSL. I think there's something wrong with the SSL certs on regnetworkweb.org.

Ok, Thanks a lot for this ! 👍 👍

@codecov-commenter
Copy link

Codecov Report

Merging #208 (4e3dd38) into master (8123f42) will increase coverage by 7.59%.
The diff coverage is 51.22%.

@@            Coverage Diff             @@
##           master     #208      +/-   ##
==========================================
+ Coverage   40.27%   47.86%   +7.59%     
==========================================
  Files          48       92      +44     
  Lines        2811     5534    +2723     
==========================================
+ Hits         1132     2649    +1517     
- Misses       1679     2885    +1206     
Impacted Files Coverage Δ
graphein/grn/parse_trrust.py 37.77% <ø> (ø)
graphein/ml/diffusion.py 0.00% <0.00%> (ø)
graphein/ppi/edges.py 100.00% <ø> (ø)
graphein/ppi/graph_metadata.py 0.00% <ø> (ø)
graphein/ppi/graphs.py 54.34% <ø> (ø)
graphein/ppi/parse_biogrid.py 75.00% <ø> (ø)
graphein/ppi/visualisation.py 0.00% <0.00%> (ø)
graphein/protein/analysis.py 0.00% <0.00%> (ø)
graphein/protein/edges/intramolecular.py 22.68% <0.00%> (ø)
graphein/protein/features/sequence/utils.py 28.00% <0.00%> (+3.00%) ⬆️
... and 80 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@a-r-j
Copy link
Owner

a-r-j commented Sep 16, 2022

Thanks for the great PR @rg314 @1511878618

@a-r-j a-r-j merged commit ae6ef9d into a-r-j:master Sep 16, 2022
@sonarcloud
Copy link

sonarcloud bot commented Sep 16, 2022

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 5 Code Smells

No Coverage information No Coverage information
40.0% 40.0% Duplication

a-r-j added a commit that referenced this pull request Sep 16, 2022
* add load local pdb_files to ProteinGraphDataset

* load local pdb_files from a list

* test and black and isort and add CHANGELOG.md

* docstring formatting

* comment jupyter; just run  and ssh or vscode to interact with container

* install dependency of jupyter nbextension

* Fixed Dockerfile and unit tests

* Update chage log

Co-authored-by: xutingfeng <xutingfeng@big.ac.cn>
Co-authored-by: Arian Jamasb <arjamasb@gmail.com>
Co-authored-by: ryan <ryan.greenhalgh@hotmail.co.uk>

Co-authored-by: Nicktf <49584439+1511878618@users.noreply.github.com>
Co-authored-by: xutingfeng <xutingfeng@big.ac.cn>
Co-authored-by: ryan <ryan.greenhalgh@hotmail.co.uk>
a-r-j added a commit that referenced this pull request Sep 16, 2022
* fix disulphide bond node filtering #205

* Local dataset (#208) (#211)

* add load local pdb_files to ProteinGraphDataset

* load local pdb_files from a list

* test and black and isort and add CHANGELOG.md

* docstring formatting

* comment jupyter; just run  and ssh or vscode to interact with container

* install dependency of jupyter nbextension

* Fixed Dockerfile and unit tests

* Update chage log

Co-authored-by: xutingfeng <xutingfeng@big.ac.cn>
Co-authored-by: Arian Jamasb <arjamasb@gmail.com>
Co-authored-by: ryan <ryan.greenhalgh@hotmail.co.uk>

Co-authored-by: Nicktf <49584439+1511878618@users.noreply.github.com>
Co-authored-by: xutingfeng <xutingfeng@big.ac.cn>
Co-authored-by: ryan <ryan.greenhalgh@hotmail.co.uk>

* fix typo

Co-authored-by: Nicktf <49584439+1511878618@users.noreply.github.com>
Co-authored-by: xutingfeng <xutingfeng@big.ac.cn>
Co-authored-by: ryan <ryan.greenhalgh@hotmail.co.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants