Filter update

[frederik-sandfort1]

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.

[frederik-sandfort1]

@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.

[JochenSiegWork]

almost there :)

[frederik-sandfort1]

Resolved most, and some comments. Please have a look

[frederik-sandfort1]

@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

[JochenSiegWork]

@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.

[c-w-feldmann]

@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]

@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.

[c-w-feldmann]

Looks good to me. @JochenSiegWork, your turn :)

[JochenSiegWork]

let's merge!