Filter update #67

frederik-sandfort1 · 2024-08-20T13:59:01Z

Closes #66 and #61 .

Additionally implements a (jsonable) DescriptorsFilter.

Also did some code cleaning and removed unnecessary lines of code without a breaking change

c-w-feldmann

This was my first round reading. Will check again tomorrow in more detail.

molpipeline/abstract_pipeline_elements/mol2mol/filter.py

molpipeline/mol2mol/filter.py

tests/test_elements/test_mol2mol/test_mol2mol_filter.py

molpipeline/mol2mol/filter.py

frederik-sandfort1 · 2024-08-22T15:20:24Z

@c-w-feldmann @JochenSiegWork

I have one mypy issue, which I do not understand.

Also three questions:

should we combine smarts and smiles filter within a single class? option usesmiles?
should we check the input patterns for valid smarts/smiles?
should we apply the same logic to ElementFilter?

c-w-feldmann

I think my head is exploding, but beside the small additions in the doc string, nothing I can complain about. @JochenSiegWork , your turn.

molpipeline/abstract_pipeline_elements/mol2mol/filter.py

JochenSiegWork

almost there :)

molpipeline/abstract_pipeline_elements/mol2mol/filter.py

molpipeline/utils/value_conversions.py

JochenSiegWork · 2024-09-11T09:00:52Z

molpipeline/mol2mol/filter.py

-        if not unique_elements.issubset(self.allowed_element_numbers):
-            forbidden_elements = unique_elements - self.allowed_element_numbers
+        to_process_value = (
+            Chem.AddHs(value) if 1 in self.allowed_element_numbers else value


Implicitly modifying the molecule does not seem right. I understand that this can make sense when you are interested in exact element counts in the molecules. However, the filter should just filter molecules and not implicitly modify them before filtering. I think the most consistent way would be to let the user explicitly add hydrogens before the filter, which would result in an additional pipeline element. However, I think a compromise would also be fine by adding a flag add_hs to the ElementFilter's constructor to let the user choose.

@c-w-feldmann what do you think?

Here I disagree... If the Use adds the element number 1 in allowed_elements, I am pretty sure he would like to get the Hydrogens counted... The returned molecules do NOT have the added hydrogens, because this creates a new mol object

You're absolutely right, and I understand your point. I have 3 aspects I am unhappy with:

It's not explicit but implicit, and I advocate the "Explicit is Better Than Implicit" principle. However, this might also be solved with good, explicit docstrings that you can add.

The molecules might already have been prepared with Hydrogens added. Calling Chem.AddHs again adds additional computation for every molecule.

I am not sure about our use cases, but for docking, for example, the users typically prepare the protonation states of their molecules with some other tool beforehand. In which case readding hydrogen might mess with the prepared molecules. Is something similar also possible in our use cases?

Good point, could we check, whether hydrogens are there already? Or should we alternatively log a warning here if hydrogens are requested but the count is 0 and tell people to e.g. use an AddHs Pipeline module? (not sure whether this exists?)

the three of us should talk about it together. This is a more conceptual decision we need to make.

The default in most cases is only implicit hydrogens, but I recognize the need that the user might want to keep the molecule as is. I would propose to add the attribute add_hydrogens, default:True, which adds Hs if Hs are counted.

I like the attribute, included this and added a test (if set to false and a count of Hydrogens is included, I print a warning, that this will yield unexpected results (because the hydrogen number is 0 in this case)).

I wanted to test the logging (I already did via debugger), but also wanted to add a capturing of the logs. I tried this: loguru log capturing, but this did not work unfortunately...

@c-w-feldmann please, check this again

molpipeline/mol2mol/filter.py

tests/test_elements/test_mol2mol/test_mol2mol_filter.py

frederik-sandfort1

Resolved most, and some comments. Please have a look

molpipeline/abstract_pipeline_elements/mol2mol/filter.py

frederik-sandfort1 · 2024-09-11T12:59:00Z

molpipeline/mol2mol/filter.py

-        if not unique_elements.issubset(self.allowed_element_numbers):
-            forbidden_elements = unique_elements - self.allowed_element_numbers
+        to_process_value = (
+            Chem.AddHs(value) if 1 in self.allowed_element_numbers else value


Here I disagree... If the Use adds the element number 1 in allowed_elements, I am pretty sure he would like to get the Hydrogens counted... The returned molecules do NOT have the added hydrogens, because this creates a new mol object

molpipeline/utils/value_conversions.py

tests/test_elements/test_mol2mol/test_mol2mol_filter.py

frederik-sandfort1 · 2024-09-12T05:36:16Z

@c-w-feldmann @JochenSiegWork I added an additional filter (ComplexFilter) which can be initiatied with multiple moltomol elements and uses the keep matches logic

I also had a look on auto2mol - I was a bit puzzled about serializability (there you also just set the elements as attributes, no get_params / set_params functionality given. Please have a look on the implementation especially regarding serializability

JochenSiegWork

just two small comments

molpipeline/utils/value_conversions.py

molpipeline/abstract_pipeline_elements/mol2mol/filter.py

molpipeline/utils/value_conversions.py

molpipeline/abstract_pipeline_elements/mol2mol/filter.py

tests/test_elements/test_mol2mol/test_mol2mol_filter.py

JochenSiegWork · 2024-09-12T10:25:37Z

@c-w-feldmann @JochenSiegWork

I have one mypy issue, which I do not understand.

Also three questions:

should we combine smarts and smiles filter within a single class? option usesmiles?

Strong yes. I think we should combine SMARTS and SMILES filter within a single class, because every SMILES is a valid SMARTS. Effectively a list of SMILES is a list of SMARTS.

should we check the input patterns for valid smarts/smiles?

No. But since you pre-compute the molecules for the SMARTS/SMILES now this is done automatically during the construction of the filter. So you already included this feature now.

should we apply the same logic to ElementFilter?

We talked about this. We can in the future because it should fit but we don't have any real use case right now. So postpone.

molpipeline/abstract_pipeline_elements/mol2mol/filter.py

JochenSiegWork · 2024-09-13T08:27:11Z

molpipeline/mol2mol/filter.py

-        if not unique_elements.issubset(self.allowed_element_numbers):
-            forbidden_elements = unique_elements - self.allowed_element_numbers
+        to_process_value = (
+            Chem.AddHs(value) if 1 in self.allowed_element_numbers else value


the three of us should talk about it together. This is a more conceptual decision we need to make.

molpipeline/mol2mol/filter.py

molpipeline/utils/value_conversions.py

molpipeline/mol2mol/filter.py

molpipeline/abstract_pipeline_elements/mol2mol/filter.py

c-w-feldmann · 2024-09-30T14:50:31Z

molpipeline/mol2mol/filter.py

-        if not unique_elements.issubset(self.allowed_element_numbers):
-            forbidden_elements = unique_elements - self.allowed_element_numbers
+        to_process_value = (
+            Chem.AddHs(value) if 1 in self.allowed_element_numbers else value


The default in most cases is only implicit hydrogens, but I recognize the need that the user might want to keep the molecule as is. I would propose to add the attribute add_hydrogens, default:True, which adds Hs if Hs are counted.

molpipeline/mol2mol/filter.py

c-w-feldmann · 2024-09-30T14:56:17Z

molpipeline/mol2mol/filter.py

+
+
+class ComplexFilter(_BaseKeepMatchesFilter):
+    """Filter to keep or remove molecules based on multiple filter elements.


I think we need to check the get and set params of the subfilters.
To ensure the correct passing of sub-parameters I am afraid that we would need to provide them as a tuple in the init:
(name, object): (lower, upper)
name: str
object: filter
lower: float| int
upper: float| int

I think we should quickly discuss on monday - I am not onehundred percent sure, how this actually helps

This would help during the get/set_params section, as the sklearn schema does not support giving parameters to objects based on their position in a list. I feel this will be needed to save/load the object in a json file. See comment to the test below.

c-w-feldmann · 2024-09-30T15:09:24Z

tests/test_elements/test_mol2mol/test_mol2mol_filter.py

+        filtered_smiles_2 = pipeline.fit_transform(SMILES_LIST)
+        self.assertEqual(filtered_smiles_2, [SMILES_BENZENE, SMILES_CHLOROBENZENE])
+
+    def test_complex_filter(self) -> None:


we also need a test_get_set_params_complex_filter.

I would advocate for implementing one TestClass for each FilterClass. @JochenSiegWork what do you think?

I just filled the existing structure, but would be good with one class per FilterClass...

What would you like the test_get_set_params_complex_filter to test exactly? Also, would like to point here where there is just the object included

The critical part is if we can write it as a json file and load the the object from the file.
A nice to have would be if the parameters of the subfilters can be accessed via the get/set_params from the filter, à la filter.set_params(subfiltername__allowed_element_numbers=[...])

How does the write as json file work (where is the code reference)? I do not see the direct relation of get/set_params and save as json (also, see the link in my previous comment, probably this is the same issue)

tests/test_elements/test_mol2mol/test_mol2mol_filter.py

c-w-feldmann · 2024-09-30T15:24:12Z

@frederik-sandfort1

smiles filter within a single class? option usesmiles?

I would prefer to keep them in separate classes.

should we check the input patterns for valid smarts/smiles?

Yes, we should check the input, and raise an Error of the mol-object is None.

should we apply the same logic to ElementFilter?

What would be an invalid element? Or do you mean the same filterlogic? I think if it was possible to filer element with the code from the BaseFilter, we should do it.

c-w-feldmann · 2024-09-30T15:28:10Z

@c-w-feldmann @JochenSiegWork
I have one mypy issue, which I do not understand.
Also three questions:

should we combine smarts and smiles filter within a single class? option usesmiles?

Strong yes. I think we should combine SMARTS and SMILES filter within a single class, because every SMILES is a valid SMARTS. Effectively a list of SMILES is a list of SMARTS.

should we check the input patterns for valid smarts/smiles?

No. But since you pre-compute the molecules for the SMARTS/SMILES now this is done automatically during the construction of the filter. So you already included this feature now.

should we apply the same logic to ElementFilter?

We talked about this. We can in the future because it should fit but we don't have any real use case right now. So postpone.

I love that I gave completely different answers than Jochen :D
Maybe we need to sit together and have a chat.

frederik-sandfort1 added 4 commits August 19, 2024 17:19

remove unnecessary inits and refactor

fa27bfd

include smarts filter, smiles filter, descriptors filter

b706268

Fix wrong typing that caused thousands of type ignores

476d65a

linting and fix element number test

f14b71a

frederik-sandfort1 self-assigned this Aug 20, 2024

frederik-sandfort1 requested review from c-w-feldmann and JochenSiegWork August 20, 2024 13:59

frederik-sandfort1 added 2 commits August 21, 2024 15:45

Merge branch 'main' into filter_update

e3f5d2d

reset name typing

c352144

c-w-feldmann reviewed Aug 21, 2024

View reviewed changes

frederik-sandfort1 added 4 commits August 22, 2024 11:56

Christians first review

5c95f81

more changes

16088db

linting

b2ca26d

pylint

81ffb7c

c-w-feldmann reviewed Aug 22, 2024

View reviewed changes

molpipeline/mol2mol/filter.py Outdated Show resolved Hide resolved

c-w-feldmann self-requested a review August 22, 2024 11:43

rewrite filter logic (#71)

9fed198

frederik-sandfort1 marked this pull request as draft August 22, 2024 12:55

Combine filters with one base logic

f49cb70

frederik-sandfort1 marked this pull request as ready for review August 22, 2024 15:19

This comment was marked as resolved.

Sign in to view

c-w-feldmann and others added 3 commits August 22, 2024 19:34

change dict to Mapping

91feed1

Merge branch 'main' into filter_update

1d70f17

isort

93e6183

c-w-feldmann requested changes Sep 10, 2024

View reviewed changes

molpipeline/abstract_pipeline_elements/mol2mol/filter.py Show resolved Hide resolved

molpipeline/abstract_pipeline_elements/mol2mol/filter.py Outdated Show resolved Hide resolved

JochenSiegWork requested changes Sep 11, 2024

View reviewed changes

Include comments

cd18310

frederik-sandfort1 commented Sep 12, 2024

View reviewed changes

linting

c0427ab

linting and ComplexFilter

cfdfd83

JochenSiegWork requested changes Sep 12, 2024

View reviewed changes

frederik-sandfort1 added 2 commits September 12, 2024 16:18

typing, tests, complex filter naming

b843657

finalize filter refactoring

a93344c

JochenSiegWork requested changes Sep 13, 2024

View reviewed changes

frederik-sandfort1 requested a review from c-w-feldmann September 30, 2024 11:29

c-w-feldmann requested changes Sep 30, 2024

View reviewed changes

frederik-sandfort1 and others added 3 commits October 1, 2024 09:17

Merge branch 'main' into filter_update

235c8f8

review Christian

47d4d90

pylint

1f8dc1c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter update #67

Filter update #67

frederik-sandfort1 commented Aug 20, 2024 •

edited

Loading

c-w-feldmann left a comment

frederik-sandfort1 commented Aug 22, 2024

This comment was marked as resolved.

c-w-feldmann left a comment

JochenSiegWork left a comment

JochenSiegWork Sep 11, 2024

frederik-sandfort1 Sep 11, 2024

JochenSiegWork Sep 12, 2024

frederik-sandfort1 Sep 12, 2024

JochenSiegWork Sep 13, 2024

c-w-feldmann Sep 30, 2024

frederik-sandfort1 Oct 1, 2024

frederik-sandfort1 left a comment

frederik-sandfort1 Sep 11, 2024

frederik-sandfort1 commented Sep 12, 2024

JochenSiegWork left a comment

JochenSiegWork commented Sep 12, 2024 •

edited

Loading

JochenSiegWork Sep 13, 2024

c-w-feldmann Sep 30, 2024

c-w-feldmann Sep 30, 2024

c-w-feldmann Sep 30, 2024

frederik-sandfort1 Oct 1, 2024

c-w-feldmann Oct 1, 2024

c-w-feldmann Sep 30, 2024

frederik-sandfort1 Oct 1, 2024

c-w-feldmann Oct 1, 2024

frederik-sandfort1 Oct 1, 2024

c-w-feldmann commented Sep 30, 2024 •

edited

Loading

c-w-feldmann commented Sep 30, 2024



		class ComplexFilter(_BaseKeepMatchesFilter):
		"""Filter to keep or remove molecules based on multiple filter elements.

Filter update #67

Are you sure you want to change the base?

Filter update #67

Conversation

frederik-sandfort1 commented Aug 20, 2024 • edited Loading

c-w-feldmann left a comment

Choose a reason for hiding this comment

frederik-sandfort1 commented Aug 22, 2024

This comment was marked as resolved.

c-w-feldmann left a comment

Choose a reason for hiding this comment

JochenSiegWork left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederik-sandfort1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frederik-sandfort1 commented Sep 12, 2024

JochenSiegWork left a comment

Choose a reason for hiding this comment

JochenSiegWork commented Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c-w-feldmann commented Sep 30, 2024 • edited Loading

c-w-feldmann commented Sep 30, 2024

frederik-sandfort1 commented Aug 20, 2024 •

edited

Loading

JochenSiegWork commented Sep 12, 2024 •

edited

Loading

c-w-feldmann commented Sep 30, 2024 •

edited

Loading