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

Example usage #4

Merged
merged 4 commits into from
Jun 1, 2023
Merged

Example usage #4

merged 4 commits into from
Jun 1, 2023

Conversation

JonasSchaub
Copy link
Owner

Hi @B-s123 ,

I have created a new test class with two methods illustrating the basic usage of the fragment fingerprinter functionality. My suggestion would be to turn those also into the basic usage given in the GitHub wiki. Could you have a look at it and tell me what you think?

Kind regards,
Jonas

Copy link
Collaborator

@FelixBaensch FelixBaensch left a comment

Choose a reason for hiding this comment

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

Overall it looks good, but please answer the questions.

while (tmpSDFReader.hasNext()) {
IAtomContainer tmpMolecule = tmpSDFReader.next();
tmpFragmenter.generateFragments(tmpMolecule);
IAtomContainer[] tmpFragments = tmpFragmenter.getFragmentsAsContainers();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the usage of getFragmentsAsContainers() and not getFragments(), which returns them already as unique SMILES?
At the end, there is no big difference. I ask out of interest.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I like to have full control via my own SmilesGenerator instance here. And I think it is also important in this context to show this explicitly. Not every fragmentation functionality will have this kind of convenience function. In addition, every other string-based representation could be used instead of SMILES.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps a brief comment will make this even clearer.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Added comment in last commit

Copy link
Collaborator

Choose a reason for hiding this comment

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

I find the example very illustrative and a very appropriate chemical example.

and i can gladly add real tests

@JonasSchaub
Copy link
Owner Author

@B-s123

I find the example very illustrative and a very appropriate chemical example.

and i can gladly add real tests

That is wonderful to hear! If you think all is fine, please approve and merge the pull request.

@FelixBaensch FelixBaensch merged commit 57c9f90 into FragmentFingerprint Jun 1, 2023
@FelixBaensch
Copy link
Collaborator

Wonderful

@FelixBaensch FelixBaensch deleted the example_usage branch June 1, 2023 13:43
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.

None yet

3 participants