Expand random_state type hints to accept RandomState by LiudengZhang · Pull Request #527 · MLCIL/scikit-fingerprints

LiudengZhang · 2026-03-10T22:23:41Z

Summary

Expanded random_state parameter type annotations from int | None to int | np.random.RandomState | np.random.Generator | None across 5 files, aligning with scikit-learn conventions
Skipped conformer_generator.py (RDKit only accepts int) and maxmin_split.py (same RDKit limitation)
randomized_scaffold_split.py already had the broad type and was left unchanged

Files changed

skfp/bases/base_fp_transformer.py
skfp/bases/base_substructure_fp.py
skfp/fingerprints/e3fp_fp.py
skfp/fingerprints/map.py
skfp/model_selection/hyperparam_search/randomized_search.py

Test plan

pytest tests/ -k "E3FP or MAP" — all 14 selected tests pass
No functional changes, type hints only

Fixes #523

j-adamczyk · 2026-03-11T09:45:57Z

Thanks for the contribution.

I think we also need tests that cover those, checking if RandomState and Generator work properly for those classes that have random_state.

Also, code style, please use np.random.RandomState rather than importing it directly, I think that's more readable. We try to use np. prefix in all places where NumPy is used.

LiudengZhang · 2026-03-11T22:31:07Z

Updated to use np.random. prefix and added np.random.Generator to the parameter constraints alongside tests for RandomState and Generator. Thanks for the feedback!

j-adamczyk

Please also add tests for other classes, i.e., randomized search and randomized scaffold split. You can also check test coverage for those files, see the Makefile for example command.

j-adamczyk · 2026-03-12T07:27:21Z

skfp/bases/base_fp_transformer.py

        "batch_size": [Integral, None],
        "verbose": ["verbose", dict],
-        "random_state": ["random_state"],
+        "random_state": ["random_state", np.random.Generator],


Now that I go through scikit-learn validation (https://github.com/scikit-learn/scikit-learn/blob/7aae3427d8b6b7ee6aa150ac355262dd0b0e793f/sklearn/utils/_param_validation.py#L557), I realized that we don't need to support np.random.Generator, just np.random.RandomState. So we can simplify this, e.g. keep only "random_state" here. Sorry for not noticing that earlier.

LiudengZhang · 2026-03-16T18:22:31Z

Good catch — removed Generator from the imports and type hints in randomized_scaffold_split.py. The _parameter_constraints already correctly use "random_state" (which maps to int/RandomState/None in sklearn's validation). Tests for all three classes (MAP fingerprint, randomized search, and randomized scaffold split) pass with int, RandomState, and None.

j-adamczyk

One last change - move tests for each class/function next to its other functions. We want to group tests by major logical functionality, rather than by their code functionality like random state. After that, PR will be ready to merge

j-adamczyk · 2026-03-16T19:29:28Z

tests/bases/test_random_state_typing.py

+@pytest.mark.parametrize(
+    "random_state",
+    [42, np.random.RandomState(42), None],
+    ids=["int", "RandomState", "None"],
+)
+def test_randomized_scaffold_split_random_state_types(random_state):
+    """randomized_scaffold_train_test_split should accept int, RandomState, or None."""
+    smiles = [
+        "C1CCCC(C2CC2)CC1",
+        "c1n[nH]cc1C1CCCCCC1",
+        "c1n[nH]cc1CC1CCCCCC1",
+        "C1CCCC(CC2CCOCC2)CC1",
+        "c1ccc2nc(OC3CCC3)ccc2c1",
+        "O=C(CCc1cscn1)NC1CCNCC1",
+        "c1ccc2nc(OC3CCOC3)ccc2c1",
+        "c1ccc2nc(NC3CCOCC3)ccc2c1",
+        "c1ccc2nc(N3CCCOCC3)ccc2c1",
+        "c1ccc2nc(N3CCn4ccnc4C3)ccc2c1",
+    ]
+    train, test = randomized_scaffold_train_test_split(
+        smiles, random_state=random_state
+    )
+    assert len(train) + len(test) == len(smiles)


Move to randomized_scaffold_train_test_split tests

j-adamczyk · 2026-03-16T19:29:37Z

tests/bases/test_random_state_typing.py

+def test_randomized_search_random_state_types(smallest_mols_list, random_state):
+    """FingerprintEstimatorRandomizedSearch should accept int, RandomState, or None."""
+    num_mols = len(smallest_mols_list)
+    y = np.concatenate([np.ones(num_mols // 2), np.zeros(num_mols - num_mols // 2)])
+
+    fp = AtomPairFingerprint()
+    fp_params = {"max_distance": list(range(2, 6))}
+    estimator_cv = GridSearchCV(
+        estimator=DummyClassifier(strategy="constant", constant=1),
+        param_grid={"constant": [0, 1]},
+        scoring="accuracy",
+    )
+    fp_cv = FingerprintEstimatorRandomizedSearch(
+        fp, fp_params, estimator_cv, n_iter=2, random_state=random_state
+    )
+    fp_cv.fit(smallest_mols_list, y)
+    assert len(fp_cv.cv_results_) == 2


Move to FingerprintEstimatorRandomizedSearch tests

j-adamczyk · 2026-03-16T19:29:53Z

tests/bases/test_random_state_typing.py

+def test_map_fp_random_state_types(smallest_smiles_list, random_state):
+    """MAPFingerprint should accept int, RandomState, or None."""
+    fp = MAPFingerprint(random_state=random_state, n_jobs=-1)
+    X = fp.transform(smallest_smiles_list)
+    assert X.shape[0] == len(smallest_smiles_list)


Move to MAPFingerprint tests

Please make the tests for other fingerprints which use random state too.

LiudengZhang · 2026-03-16T19:37:48Z

Done — moved each test next to its class/function tests: test_map_fp_random_state_types into tests/fingerprints/map.py, test_randomized_search_random_state_types into tests/model_selection/hyperparam_search/randomized_selection.py, and test_randomized_scaffold_split_random_state_types into tests/model_selection/splitters/randomized_scaffold_split.py. Deleted the standalone test_random_state_typing.py. All 9 tests pass.

mjste

Please add tests for fingerprints other than map that utilize randomness.

LiudengZhang · 2026-03-18T07:02:53Z

Added random_state type tests for E3FPFingerprint in 99668fc. That should cover all fingerprints that use randomness (MAP and E3FP).

LiudengZhang · 2026-03-18T17:14:56Z

Added E3FP random_state type tests in 99668fc. MAP and E3FP are the only two fingerprints that accept random_state, so that should cover all of them. Let me know if I missed any!

j-adamczyk

Please use random seed 0 in tests, we use that convention everywhere
There is a merge conflict that needs to be resolved

After those, I want to merge this PR

j-adamczyk

One more thing - docstrings should state int, RandomState instance or None to follow scikit-learn

Align random_state parameter type annotations with scikit-learn conventions by accepting np.random.RandomState and np.random.Generator in addition to int and None. Fixes MLCIL#523

Per review: sklearn validation only supports RandomState, not Generator. Reverted _parameter_constraints to ["random_state"] (no Generator). Type hints now use int | RandomState | None (no Generator). Added parametrized tests for FingerprintEstimatorRandomizedSearch and randomized_scaffold_train_test_split.

sklearn's random_state constraint only accepts int, RandomState, or None — Generator is not supported. Remove it from imports and type hints in randomized_scaffold_split.py.

Group tests by logical functionality as requested. Moved from standalone test_random_state_typing.py into map.py, randomized_selection.py, and randomized_scaffold_split.py.

Use random seed 0 (repo convention) in all random_state type tests. Update docstrings to follow scikit-learn convention: "int, RandomState instance or None".

j-adamczyk · 2026-03-23T20:41:41Z

@LiudengZhang we saw that fixes have been pushed and tests passed, so I'm merging this. Thanks for the contribution!

LiudengZhang requested review from j-adamczyk, mjste and my-alaska as code owners March 10, 2026 22:23

j-adamczyk reviewed Mar 12, 2026

View reviewed changes

j-adamczyk requested changes Mar 16, 2026

View reviewed changes

j-adamczyk previously approved these changes Mar 16, 2026

View reviewed changes

mjste requested changes Mar 17, 2026

View reviewed changes

LiudengZhang dismissed j-adamczyk’s stale review via 99668fc March 18, 2026 07:02

j-adamczyk requested changes Mar 22, 2026

View reviewed changes

j-adamczyk changed the title ~~Expand random_state type hints to accept RandomState and Generator~~ Expand random_state type hints to accept RandomState Mar 22, 2026

j-adamczyk requested changes Mar 22, 2026

View reviewed changes

LiudengZhang added 7 commits March 23, 2026 13:46

Expand random_state type hints to accept RandomState and Generator

0486e3a

Align random_state parameter type annotations with scikit-learn conventions by accepting np.random.RandomState and np.random.Generator in addition to int and None. Fixes MLCIL#523

Address review: add Generator to param constraints and tests

1b57cfb

drop Generator from randomized scaffold split type hints

1297ca6

sklearn's random_state constraint only accepts int, RandomState, or None — Generator is not supported. Remove it from imports and type hints in randomized_scaffold_split.py.

move random_state tests next to their class/function tests

e14ed41

Group tests by logical functionality as requested. Moved from standalone test_random_state_typing.py into map.py, randomized_selection.py, and randomized_scaffold_split.py.

Add random_state type tests for E3FPFingerprint

cbed2a7

address review: use seed 0 in tests and update docstrings

069beea

Use random seed 0 (repo convention) in all random_state type tests. Update docstrings to follow scikit-learn convention: "int, RandomState instance or None".

LiudengZhang force-pushed the feat/random-state-typing branch from 99668fc to 069beea Compare March 23, 2026 18:48

j-adamczyk approved these changes Mar 23, 2026

View reviewed changes

mjste approved these changes Mar 23, 2026

View reviewed changes

j-adamczyk merged commit 0aa3f28 into MLCIL:master Mar 24, 2026
13 checks passed

Conversation

LiudengZhang commented Mar 10, 2026

Summary

Files changed

Test plan

Uh oh!

j-adamczyk commented Mar 11, 2026

Uh oh!

LiudengZhang commented Mar 11, 2026

Uh oh!

j-adamczyk left a comment

Choose a reason for hiding this comment

Uh oh!

j-adamczyk Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

LiudengZhang commented Mar 16, 2026

Uh oh!

j-adamczyk left a comment

Choose a reason for hiding this comment

Uh oh!

j-adamczyk Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

j-adamczyk Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

j-adamczyk Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

mjste Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

LiudengZhang commented Mar 16, 2026

Uh oh!

mjste left a comment

Choose a reason for hiding this comment

Uh oh!

LiudengZhang commented Mar 18, 2026

Uh oh!

LiudengZhang commented Mar 18, 2026

Uh oh!

j-adamczyk left a comment

Choose a reason for hiding this comment

Uh oh!

j-adamczyk left a comment

Choose a reason for hiding this comment

Uh oh!

j-adamczyk commented Mar 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants