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

add sanitize arg in read_sdf #38

Merged
merged 5 commits into from May 11, 2021
Merged

Conversation

Ishan-Kumar2
Copy link
Contributor

Checklist:

  • Add tests to cover the fixed bug(s) or the new introduced feature(s) (not needed).
  • Added a news entry.
    • copy news/TEMPLATE.rst to news/my-feature-or-branch.rst) and edit it.

@hadim
Copy link
Contributor

hadim commented May 10, 2021

Thank you @Ishan-Kumar2 for your contribution!

@maclandrol do you think it's usefull to propagate strictParsing as well?

@maclandrol
Copy link
Member

maclandrol commented May 10, 2021

@maclandrol do you think it's usefull to propagate strictParsing as well?

Aside from not validating the SDF definition, I am struggling to see use cases where strictParsing!=True would be useful.

^ On second thought, yes.

@Ishan-Kumar2
Copy link
Contributor Author

Ishan-Kumar2 commented May 11, 2021

Thanks for the feedback @maclandrol and @hadim
I have added the strictParsing arg and used the dm function for santisation.

I just checked that the sanitize_mol is causing the output to have a different address as the original molecule. This causes the GetPropsAsDict() function to not work and hence some of the tests fail. Is there a fix through which we can keep the old addresses?

Copy link
Contributor

@hadim hadim left a comment

Choose a reason for hiding this comment

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

Good catches, the props are lost during sanitize_mol. You could use dm.copy_mol_props or dm.set_mol_props.

datamol/io.py Outdated Show resolved Hide resolved
datamol/io.py Show resolved Hide resolved
@hadim hadim self-requested a review May 11, 2021 15:51
@hadim hadim requested a review from maclandrol May 11, 2021 15:58
@hadim
Copy link
Contributor

hadim commented May 11, 2021

I will let @maclandrol doing a last review and it should be fine.

Tests are failing because of the conflict in the doc dependencies not related to this PR. I just fixed it on master. @Ishan-Kumar2 can you rebase this PR on master and check whether the tests are passing or not?

@hadim hadim linked an issue May 11, 2021 that may be closed by this pull request
datamol/io.py Outdated Show resolved Hide resolved
Copy link
Member

@maclandrol maclandrol left a comment

Choose a reason for hiding this comment

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

See comment on sanitize_mol. Otherwise lgtm

datamol/io.py Show resolved Hide resolved
@maclandrol
Copy link
Member

Thanks @Ishan-Kumar2 for the awesome contribution !

@hadim
Copy link
Contributor

hadim commented May 11, 2021

Thanks for your contribution!!!

@hadim hadim merged commit 54df661 into datamol-io:master May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add sanitize arg to read_sdf
3 participants