Fix use count based fp - #75
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the default fingerprint-generation behavior to use count-based fingerprints by default, and adjusts Morgan fingerprint defaults to better align with the framework’s configured fingerprint parameter defaults.
Changes:
- Switch default
use_countstoTrueforFingerprintsGenericandMorganFingerprints. - Increase
MorganFingerprintsdefaultfpSizeto2048. - Switch
FeatureGenerationConfig.use_countsdefault toTrue, affecting the default feature-generation pipeline behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/mother/feature_generation/core.py | Changes default fingerprint behavior (count vs. binary) and Morgan fingerprint default size/params. |
| src/mother/feature_generation/config.py | Changes the default config setting that controls whether count fingerprints are used in the feature-generation pipeline. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/mother/feature_generation/core.py:70
- The public docs still state that fingerprints default to binary output (use_counts=False) and show MorganFingerprints examples with fpSize=1024. With this PR changing the defaults to use_counts=True (and MorganFingerprints fpSize=2048), mkdocs/docs/feature_generation.md becomes misleading and should be updated to reflect the new default behavior (and ideally update the examples/table accordingly).
def __init__(self, fp_type: str, parameters: dict, use_counts: bool = True) -> None:
self.fp_type: str = fp_type
self.parameters: dict = parameters
self.use_counts: bool = use_counts
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
src/mother/ml/models/m_tabpfn.py:386
- Same scope concern as above: this PR is about defaulting Morgan fingerprints to count-based, but it also changes TabPFNClassifierMother defaults by setting
inference_precision=torch.float32. Please either document this in the PR description (and any user-facing docs) or move it to a separate PR.
# TabPFNClassifier.create_default_for_version(ModelVersion.V2)
if "model_path" not in kwargs:
kwargs["model_path"] = TabPFNClassifier.create_default_for_version(ModelVersion.V2).model_path
kwargs.setdefault("inference_precision", torch.float32)
src/mother/feature_generation/config.py:143
- The default
use_countshas been changed to True here, but the user documentation still states that fingerprints are binary by default and shows examples usingMorganFingerprints(..., fpSize=1024)(see mkdocs/docs/feature_generation.md:34-39, 50, 91-92). This will mislead users after this change; please update the docs/examples to reflect count-based defaults and the 2048-bit Morgan default (and optionally mention the breaking-behavior change).
fingerprints: List[Dict[str, Any]] = Field(default=[], description="List of fingerprint generator settings")
maccs: bool = Field(default=False, description="Flag if maccs fingerprints should be generated")
chemical_descriptors: Optional[ChemicalDescriptorsParams] = Field(default=None)
use_counts: bool = Field(default=True, description="Whether to use count fingerprints")
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (1) — in code that hasn't changed since the last review.
test/unit/test_use_counts.py:129
- The PR also changes
MorganFingerprintsdefaultfpSizeto 2048, but the “default” test currently passesfpSize=2048explicitly. That means a regression in the default size wouldn’t be caught by this suite.
def test_morgan_count_default(self, mols_with_repeated_substructures):
"""MorganFingerprints with default use_counts=True should produce count output."""
fg = MorganFingerprints(radius=2, fpSize=2048)
fg.fit()
src/mother/feature_generation/core.py:132
MorganFingerprints.__init__accepts**kwargs(and callers pass many generator options viaMorganFingerprintsParams().model_dump()), but those kwargs are currently ignored. As a result, settings likecountSimulation,useBondTypes,includeChirality, etc. have no effect when using theMorganFingerprintsconvenience class, which is especially problematic now that count-based fingerprints are the default.
def __init__(
self, radius: int = 2, fpSize: int = 2048, include_chirality: bool = False, use_counts: bool = True, **kwargs
) -> None:
super().__init__(
"MorganFP",
{
"radius": radius,
"fpSize": fpSize,
"includeChirality": include_chirality,
},
use_counts=use_counts,
)
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: thomasATbayer <105632614+thomasATbayer@users.noreply.github.com>
Co-authored-by: thomasATbayer <105632614+thomasATbayer@users.noreply.github.com>
Co-authored-by: thomasATbayer <105632614+thomasATbayer@users.noreply.github.com>
Co-authored-by: thomasATbayer <105632614+thomasATbayer@users.noreply.github.com>
ae4bea6 to
833e2d9
Compare
|
closes #75 |
| def __init__( | ||
| self, radius: int = 2, fpSize=1024, include_chirality: bool = False, use_counts: bool = False, **kwargs | ||
| self, radius: int = 2, fpSize: int = 2048, include_chirality: bool = False, use_counts: bool = True, **kwargs | ||
| ) -> None: |
| def __init__( | ||
| self, radius: int = 2, fpSize=1024, include_chirality: bool = False, use_counts: bool = False, **kwargs | ||
| self, radius: int = 2, fpSize: int = 2048, include_chirality: bool = False, use_counts: bool = True, **kwargs | ||
| ) -> None: | ||
| super().__init__( |
https://github.com/Bayer-Group/MotherML/blob/main/src/mother/feature_generation/core.py https://github.com/Bayer-Group/MotherML/blob/main/src/mother/feature_generation/config.py currently the Morgan Fingeprint setting in Mother is to not use counts. Binary fingerprints are usually not a good choice for ML, so should make sure that the standard setting is to use counts. This also enables better treatment in the case of bit colission, , as they at least increase the count by +1 instead od just ignoring. And maybe increase fpsize to 2048 in core.py too asis already in the config. Most users will not change on their own, and for some tasks using binary FP can make models perform substantially worse, which is also backed by literature: Count-Based Morgan Fingerprint: A More Efficient and Interpretable Molecular Representation in Deve… Count your bits: fingerprint benchmarking to assess broad chemical space representation | Journal o…