Skip to content

Conversation

akuroiwa
Copy link
Contributor

@akuroiwa akuroiwa commented Sep 9, 2024

Dear Global-Chem developers,

I'd like to propose an update to the Morgan fingerprint generation method used in the project. This change is necessary to address a deprecation warning and to align with RDKit's modern best practices.

Current Implementation

The current implementation uses the deprecated GetMorganFingerprintAsBitVect function:

fp = AllChem.GetMorganFingerprintAsBitVect(mol, self.morgan_radius, nBits=self.bit_representation)

Proposed Change

I suggest updating the code to use the modern MorganGenerator class:

from rdkit.Chem import rdFingerprintGenerator
morgan_gen = rdFingerprintGenerator.GetMorganGenerator(radius=self.morgan_radius, fpSize=self.bit_representation)
fp = morgan_gen.GetFingerprint(mol)

Rationale

  1. Deprecation Warning: The current method triggers a deprecation warning, indicating that it will be removed in future versions of RDKit.

  2. Performance: The new MorganGenerator class is optimized for better performance, especially when generating multiple fingerprints.

  3. Consistency: This change will make the codebase consistent with modern RDKit practices and easier to maintain in the future.

  4. Flexibility: The new API offers more flexibility and options for fingerprint generation, which could be beneficial for future enhancements.

Scope of Changes

I've identified several instances where this change should be applied throughout the codebase. A comprehensive search and replace operation would be necessary to update all occurrences.

Verification

I've tested this change on a small scale and found it to produce equivalent results to the current implementation. However, I recommend a thorough verification process across all use cases in the Global-Chem project to ensure full compatibility.

References

  1. Modern RDKit Fingerprint Generation
  2. RDKit Blog: Fingerprint Generator Tutorial

I'm happy to assist with implementing and testing these changes if needed. Please let me know if you need any further information or clarification.

Thank you for considering this proposal.

Best regards,
Akihiro Kuroiwa

@akuroiwa akuroiwa requested a review from Sulstice as a code owner September 9, 2024 10:34
@Sulstice
Copy link
Collaborator

Go ahead and make all the changes where you see fit, add it to the PR and I can schedule a new release. I was thinking of decomissioning this part of the package but it seems like you guys are using it.

@ANUGAMAGE stage this for the 2.0 release.

@Sulstice Sulstice requested a review from ANUGAMAGE September 13, 2024 04:42
@Sulstice Sulstice added this to the v2.0 milestone Sep 13, 2024
@Sulstice Sulstice added bug Something isn't working 2.0 labels Sep 13, 2024
@akuroiwa
Copy link
Contributor Author

Thank you for your feedback on my pull request. I appreciate your guidance and the opportunity to make necessary adjustments.

Based on my understanding, I believe there are only three areas that require modifications. However, I noticed that a bug label has been attached to the PR. Could you please clarify what specific error outputs or issues have been encountered? This information would be very helpful for me to address any potential problems effectively.

I also wanted to mention that I'm currently developing a project called chem-ant, which imports and utilizes global_chem_extensions. The package has been extremely helpful in my work, and I'm grateful for its functionality. This is one of the reasons I'm keen on contributing to its improvement and maintenance.

Thank you once again for your support and for maintaining such a useful package. I look forward to your response.

@Sulstice
Copy link
Collaborator

Sulstice commented Sep 25, 2024

Alright @akuroiwa that is good to know because I didn't realize it was useful for folk. What I will do is bring back the code again into the repository and continue maintenance in it's development. I was about to actually yank it.

Running our base system check now.

@Sulstice
Copy link
Collaborator

@ANUGAMAGE Can you test this branch?

@Sulstice Sulstice requested a review from Lyq322 September 25, 2024 20:36
Copy link
Collaborator

@ANUGAMAGE ANUGAMAGE left a comment

Choose a reason for hiding this comment

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

I compared Modern Morgan fingerprint generation with the older method and it gave similar results with a more effective manner.

link : https://colab.research.google.com/drive/1R_ATFUKAI57XuNVhJG7jOmPgRe8rCPd9#scrollTo=xGPA3duBozvr

@Sulstice Sulstice merged commit 9c2efb9 into Global-Chem:development Oct 3, 2024
0 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2.0 bug Something isn't working

Projects

Development

Successfully merging this pull request may close these issues.

3 participants