-
Notifications
You must be signed in to change notification settings - Fork 6
Fixes and improvements to VN-EGNN restructure #11
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
Open
tobias-nolz
wants to merge
13
commits into
ml-jku:master
Choose a base branch
from
tobias-nolz:master
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ocket.sh The script path was somehow linked to BASE="data" which didn't make sense. Now it uses the PROJECT_ROOT
…gs.py call in process_data.sh -b is used for batch_size in this script, hence, the option doesn't make sense here
…res.py "-p" arg was replaced by "-i" which is used by protein_feature.py; "-b" was removed as it's not part of protein_feature.py
Please review this commit carefully. In opinion, from the given README.md, the data should be stored in `vnegnn/data/data/` for experiment=VNEGNN whereas the data for experiment=equipocket should be stored in `vnegnn/data/equipocket`. This commit adapts the paths in the preprocessing scripts as well as the hydro config accordingly.
…ders as defined in the README
Author
|
I wanted to add that |
Collaborator
|
Hi Tobias, thanks for your pull-request, will try to have a look at it in the next, maybe I set some paths wrong as I prepared the repository for GitHub. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi, while working through the setup instructions on the VNEGNN repo, I noticed that some steps are a bit unclear / not working on my side. I documented the changes I did locally and provide them to you as a pull request to review them.
The pull request includes
process_data.shandprocess_data_equipocket.sh)Updates to the data scripts
From what I could understand from the README and the provided hydra config, the data should be setup in the way as shown in the data-structure tree I added to the README (see here).
In case this is correct, some paths in the setup scripts and the hydra config were incorrect. In either case, if I follow the original README, download the provided datasets to
data/dataand execute the scripts, it will fail. Also,python src/train.py experiment=vnegnnexpects the data to be indata/equipocket(which I would have imagined forpython src/train.py experiment=equipocketinstead).I updated the paths in the scripts and hydra config accordingly.
Please let me know how it was originally intended so I can reproduce the experiments.