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.

Sign up for GitHub

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

Jump to bottom

add iupac goldbook #188

Merged
merged 15 commits into from
May 16, 2023
Merged

add iupac goldbook #188

merged 15 commits into from
May 16, 2023

Conversation

MicPie
Copy link
Contributor

@MicPie MicPie commented Apr 19, 2023

No description provided.

@MicPie MicPie requested a review from kjappelbaum April 19, 2023 14:14
@MicPie MicPie self-assigned this Apr 19, 2023
@MicPie MicPie mentioned this pull request Apr 27, 2023
Open
@kjappelbaum
Copy link
Collaborator

@MicPie is there still something you wanted to do? Otherwise, I'd add the text type and merge it

@kjappelbaum kjappelbaum mentioned this pull request May 5, 2023
Open
@MicPie MicPie marked this pull request as ready for review May 5, 2023 12:52
@MicPie MicPie requested a review from kjappelbaum May 5, 2023 12:53
kjappelbaum
kjappelbaum requested changes May 5, 2023
View reviewed changes
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.

Did you try to validate? I think it should have failed with OTHER?

MicPie and others added 2 commits May 8, 2023 09:21
@MicPie @kjappelbaum
Update data/iupac_goldbook/transform.py
f5ee295 
Co-authored-by: Kevin M Jablonka <32935233+kjappelbaum@users.noreply.github.com>
@MicPie @kjappelbaum
Update data/iupac_goldbook/meta.yaml
d97f98c 
Co-authored-by: Kevin M Jablonka <32935233+kjappelbaum@users.noreply.github.com>
@MicPie
Copy link
Contributor Author

MicPie commented May 8, 2023 •
edited
Loading

@MicPie todo: change to text in https://github.com/OpenBioML/chemnlp/blob/main/src/chemnlp/data_val/model.py#L60
Edit: We keep string. Changed everything back, which is why this PR is a little bit messy.

kjappelbaum
kjappelbaum reviewed May 16, 2023
View reviewed changes
data/chebi_20/meta.yaml
@@ -14,6 +14,9 @@ identifiers:
- id: SMILES
type: SMILES
description: SMILES
- id: compound_id
type: Other
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
type: Other
type: Other
sample: false

kjappelbaum
kjappelbaum reviewed May 16, 2023
View reviewed changes
data/chebi_20/transform.py
@@ -28,6 +28,11 @@
"type": "SMILES", # can be "SMILES", "SELFIES", "IUPAC", "OTHER"
"description": "SMILES", # description (optional, except for "OTHER")
},
{
"id": "compound_id",
"type": "Other",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"type": "Other",
"sample": False,
"type": "Other",

@kjappelbaum
Copy link
Collaborator

validation should probably work when merged in main

@kjappelbaum kjappelbaum merged commit d176b0a into main May 16, 2023
@kjappelbaum kjappelbaum deleted the goldbook_iupac branch May 16, 2023 16:40
@phalem phalem mentioned this pull request Jun 27, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kjappelbaum kjappelbaum kjappelbaum approved these changes

Assignees

@MicPie MicPie

Labels
dataset generative task needs-review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@MicPie @kjappelbaum