Feature/scikit fingerprints

[Hrovatin]

See #359

[CLAassistant]

All committers have signed the CLA.

[Scienfitz]

first round of comments

[Scienfitz]

I think we should add optional paramters here that might affect the fingerprint
for isntance bit length and radius

these could become attributes of the substanceparameter class

they should simply be ignored if a fingperirnt is selected that does not use this info

[Hrovatin]

added kwargs, up to user to specify them

[Scienfitz]

looks reasonable to me

did you add a test? Should perhaps add a parameterized test with 2 different kwargs x 3 fingerprints or similar

[Hrovatin]

added to test_fingeprints.py - test a few different params passed for fp computation. All FP names are tested in hypothesis parameters strategy substance_parameters

[Scienfitz]

For the future: I think theres no need for the fork, I added you as member, I think working directly on branches here has some advantages such that others can check out your branch seamlessly

[Hrovatin]

Add tests for all fingerprints as some are not tested yet

[Hrovatin]

@Scienfitz do you think we need to adapt this or would be what I added to test_fingerprints.py enough - just testing that all can be run from smiles to embedding?

[Scienfitz]

a bit confused about the commen being on this fiel simulate_telemetry

as for fingerprint parameterization: once you wrote a test you can simply parameterize it via pytest. For instance I can imagine a tets looping over i) all fingperrints available, ie parameterized via SubstanceEncoding so it automatically always runs over all valdi choices - with default kwargs etc PLUS ii) some selected fps like ECFP run with special kwargs to also test different nbits and radii

[Hrovatin]

i) SubstanceEncoding values are samples in hypothesis_strategies/parameters.py
ii) Added a few tests with different fp kwargs in tests/test_fingeprints.pt - done on ECFP example

[Hrovatin]

@Scienfitz I made first round of changes covering all of the above
What is still missing is figuring ot where NotEnoughPointsLeftError originates from

EDIT:
The NotEnoughPointsLeftError is also resolved now, was associated with test param naming

[AdrianSosic]

Hi @Hrovatin, thanks for taking care of this item. Haven't had the time yet to go fully over it (and probably won't before I leave). But here at least a few points from my first glance

[AdrianSosic]

I think the renaming should rather happen in the importing file, not here

[Scienfitz]

why is this even renamed?

[Hrovatin]

I added it just to be clear where they are coming from, can remove renaming

[AdrianSosic]

Haven't yet quite looked in detail at the logic, but this by far the longest converter I've seen 😄 There must be a more elegant / readable way

[Hrovatin]

I think the others are shorter for enums as names equal values, but this enum has currently different names and values, which requires these checks. But feel free to suggest how to adapt

[Scienfitz]

perhaps have a try to let chatgpt compress this to the max, but overall I dont think this is terribly long (the situation is simply more complex as with converters used elsewhere with enums)
other than that feel free to resolve

[Hrovatin]

not relevant any more as enum and hence this field was changed

[AdrianSosic]

Usually, we use UPPER_CASE writing for enum values (see other encodings, for example). Also, not sure if the "Fingerprint" at the end is really necessary? If possible, I'd rather sync the enum field name with its value

[Hrovatin]

I wanted to map parameters (currently) enum values to class names used for scikit-fingerprint (enum names) so that they can be directly converted by SubstanceParameter. I can probably switch the two (but need to test if that breaks sth else).

[Scienfitz]

if this correspondence is required its fine for me

[Hrovatin]

Now the SubstanceEncoding enum follows this rule, but there is a new enum with mapping to skfp classes (added for the new helper function that converts fp names to cls+params

[Scienfitz]

I would encourage you to perhaps write one or two more tests to really check the new functionality like kwargs, deprecation etc

[Scienfitz]

if this correspondence is required its fine for me

[Scienfitz]

@Hrovatin It would be great if, as a final sanity check, you could run the examples/backtesting/full_lookup.py example and post the resulting picture. Ive just done so on your fork and there are two observations

i) results improve which is fantastic
ii) theres also somethign super weird with the rdkit curve

can you confirm?

[Scienfitz]

@Hrovatin once the open comments are tended to we can mark this PR as ready for review

As last step before doing so: please i) make a copy of this branch as backup and then ii) merge main into this to update all the other changes that came into the repo since you started the fork. form the looks of it it doesnt seem many conflcits so that merge should be ok. Due to the amount of changes I thing rebasing is not an option but if you make backup branches you could also try that

[Hrovatin]

@Scienfitz for RDKIt there is some ruggedness where it flattens off

I checked the encoding statistics and RDKit does not seem to differ from ECFP in their distn.

N of features:

Here are mean and variance for each feature:

[Hrovatin]

@Scienfitz I made the merge. Please check comments that are still open and lmk if I should change sth or close them if ok with you

[Hrovatin]

With tox -e fulltest-py312 I get - any ideas?

_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <linear_operator.operators.matmul_linear_operator.MatmulLinearOperator object at 0x374c8f770>, eigenvectors = True, return_evals_as_lazy = False

    def _symeig(
        self: Float[LinearOperator, "*batch N N"],
        eigenvectors: bool = False,
        return_evals_as_lazy: Optional[bool] = False,
    ) -> Tuple[Float[Tensor, "*batch M"], Optional[Float[LinearOperator, "*batch N M"]]]:
        r"""
        Method that allows implementing special-cased symeig computation. Should not be called directly
        """
        from linear_operator.operators.dense_linear_operator import DenseLinearOperator
    
        if settings.verbose_linalg.on():
            settings.verbose_linalg.logger.debug(f"Running symeig on a matrix of size {self.shape}.")
    
        # potentially perform decomposition in double precision for numerical stability
        dtype = self.dtype
>       evals, evecs = torch.linalg.eigh(self.to_dense().to(dtype=settings._linalg_dtype_symeig.value()))
E       torch._C._LinAlgError: linalg.eigh: (Batch element 0): The algorithm failed to converge because the input matrix is ill-conditioned or has too many repeated eigenvalues (error code: 2).

.tox/fulltest-py312/lib/python3.12/site-packages/linear_operator/operators/_linear_operator.py:892: _LinAlgError

The above exception was the direct cause of the following exception:

campaign = Campaign(searchspace=SearchSpace(discrete=SubspaceDiscrete(parameters=(CategoricalParameter(name='Categorical_1', _val... A            OK         2.0   22.748903        1    1.0, _cached_recommendation=Empty DataFrame
Columns: []
Index: [])
n_iterations = 3, batch_size = 3

    @pytest.mark.slow
    @pytest.mark.parametrize(
        "kernel", valid_kernels, ids=[c.__class__ for c in valid_kernels]
    )
    @pytest.mark.parametrize("n_iterations", [3], ids=["i3"])
    def test_kernels(campaign, n_iterations, batch_size):
>       run_iterations(campaign, n_iterations, batch_size)

tests/test_iterations.py:243: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

rs = <RetryCallState 13644971472: attempt #5; slept for 0.0; last result: failed (_LinAlgError linalg.eigh: (Batch element ... failed to converge because the input matrix is ill-conditioned or has too many repeated eigenvalues (error code: 2).)>

    def exc_check(rs: "RetryCallState") -> None:
        fut = t.cast(Future, rs.outcome)
        retry_exc = self.retry_error_cls(fut)
        if self.reraise:
            raise retry_exc.reraise()
>       raise retry_exc from fut.exception()
E       tenacity.RetryError: RetryError[<Future at 0x374c696d0 state=finished raised _LinAlgError>]

[Scienfitz]

@Hrovatin
the example picture is off, the curve for RDKIT is wrong, seem the batch size was influenced... something very strange here is going on. Ive noticed this in one of my very earliest tests too, before looking at the ruggedness etc the curve needs to be fixed

You can ignore the _LinAlgError , it is one of the exceptions. Some of our parameter combinations sometimes seem to cause numerical instabilities that even after several repeats with different random seeds will fail. This happens randomly, you can ignore it for the ourpose of this PR (it is often fixed by re-running the failed job anyway)

A second error you can ignore in the CI is that soemtimes serializations seem to fail, without a good indication as to why. This does not hapen lcoally oin clean environments and likely is an artifact of packages, the agent etc, if it happens in your PR ignore it

[Hrovatin]

@Hrovatin the example picture is off, the curve for RDKIT is wrong, seem the batch size was influenced... something very strange here is going on. Ive noticed this in one of my very earliest tests too, before looking at the ruggedness etc the curve needs to be fixed

When just running SubstanceParameter(name=chem_type, data=data, encoding=encoding).comp_df with RDKit and the molecules from the example the shape of samples in the output df is correct. Any other ideas where this could be coming from?