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

feat: implement benchmark field 2 #128

Merged
merged 22 commits into from
Mar 23, 2023
Merged

feat: implement benchmark field 2 #128

merged 22 commits into from
Mar 23, 2023

Conversation

MicPie
Copy link
Contributor

@MicPie MicPie commented Mar 22, 2023

New PR based on PR #116 and issue #115 due to updated main branch.
PR #116 will be closed when this PR is finished.

@MicPie MicPie requested a review from kjappelbaum March 22, 2023 15:36
@MicPie MicPie self-assigned this Mar 22, 2023
@MicPie MicPie added enhancement New feature or request dataset labels Mar 22, 2023
@MicPie MicPie marked this pull request as ready for review March 22, 2023 16:05
@jackapbutler
Copy link
Collaborator

Hey @MicPie, for this use case of #108 will we continue to use split_col going forward but this denotes the split of the dataset not split in benchmarks?

@kjappelbaum
Copy link
Collaborator

Hey @MicPie, for this use case of #108 will we continue to use split_col going forward but this denotes the split of the dataset not split in benchmarks?

I think the meta.yaml are not supposed to say anything about the split we will use yet. The only thing we try to capture is what molecules have been part of a test set such that we might be able to also put them into our test set. Hence, I think this benchmark field is good enough for now (?)

@kjappelbaum
Copy link
Collaborator

@MicPie should I commit the fix for the 403 error directly to your PR?

@MicPie
Copy link
Contributor Author

MicPie commented Mar 23, 2023

@MicPie should I commit the fix for the 403 error directly to your PR?

@kjappelbaum Yes, please, feel free to add to this branch.

@jackapbutler
Copy link
Collaborator

Hey @MicPie, for this use case of #108 will we continue to use split_col going forward but this denotes the split of the dataset not split in benchmarks?

I think the meta.yaml are not supposed to say anything about the split we will use yet. The only thing we try to capture is what molecules have been part of a test set such that we might be able to also put them into our test set. Hence, I think this benchmark field is good enough for now (?)

Ah cool, so if we find one with some existing train/val/test splits should we recombine it into one for uploading?

@MicPie
Copy link
Contributor Author

MicPie commented Mar 23, 2023

Ah cool, so if we find one with some existing train/val/test splits should we recombine it into one for uploading?

@jackapbutler Yes, I would go for that. With that we can still filter it down later depending on our benchmarking setup.

Copy link
Collaborator

@kjappelbaum kjappelbaum left a comment

Choose a reason for hiding this comment

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

If the checks pass now, it is OK to merge for me. Thanks! 💯

@kjappelbaum kjappelbaum merged commit b2ae8dc into main Mar 23, 2023
@kjappelbaum kjappelbaum deleted the feat_benchmark_field_2 branch March 23, 2023 14:50
@kjappelbaum kjappelbaum mentioned this pull request Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dataset enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants