-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add SMILES standardizer code #23
Conversation
I think there is some html leaking on the PR , such as " <style scoped> .dataframe tbody tr th:only-of-type { vertical-align: middle; } ". I am going to presume that is not relevant to the PR itself. |
Yes you can ignore the leak |
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.
It requires some minor adjustments, but looks okay to me. Let me know if you think some of my comments are too cumbersome to implement. Also, I was able to install all dependencies using pip and lock them (See 0e53775, it uses poetry though). Would you consider adding that as an alternative dependency management solution? Space is already very limited on DGX and conda is well known for its massive venv sizes.
libs/smiles/test/test_data/smiles_data/JUMP-Target-2_compound_metadata_trimmed_input.tsv
Outdated
Show resolved
Hide resolved
...st/test_data/smiles_data/JUMP-Target-2_compound_metadata_trimmed_output_jump_alternate_1.csv
Outdated
Show resolved
Hide resolved
...test/test_data/smiles_data/JUMP-Target-2_compound_metadata_trimmed_output_jump_canonical.csv
Outdated
Show resolved
Hide resolved
@afermg Thank you for your thorough review! I'll go ahead and merge now. |
conda
becauserdkit
is not on pypi.gitignore
because I wasn't sure how you want to handle some ignores globally @afermgjump_canonical
corresponds to the SMILES made public via Add SMILES to compounds.csv.gz jump-cellpainting/datasets#103 (comment) (except for 1 compound; see below)