-
Notifications
You must be signed in to change notification settings - Fork 83
Added op3loader #926
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
base: main
Are you sure you want to change the base?
Added op3loader #926
Conversation
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.
Thanks for the great work!
I have added comments about a few things that need to be fixed, primarily op3_loader.sh
params_file="/tmp/datasets_openproblems_neurips2022_params.yaml" | ||
|
||
## TODO: fill in the parameters below | ||
echo "TODO: fill in the parameters below" && exit 1 |
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.
this had to be fixed/removed
|
||
set -e | ||
|
||
params_file="/tmp/datasets_openproblems_neurips2022_params.yaml" |
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.
this is about another dataset
echo "TODO: fill in the parameters below" && exit 1 | ||
|
||
cat > "$params_file" << 'HERE' | ||
param_list: |
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.
this concerns another dataset, needs to be updated with information from our dataset, e.g. from the perturbation prediction repo
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.
lgtm! Thank you for adding it!
@rcannood unless you see more issues, we could test it and merge
@rcannood, I also have some questions. Would it be possible for you to clarify them?
|
Not sure what went wrong here, but bash commands shouldn't include colons. So this:
Should be:
Indeed, this should be
Sure, that would help!
Depends on the needs of the dataset loader. If
This can be solved by updating |
The continuation of Paulos' PR:
#921