hallucination classifier and helper fixes#211
Conversation
| ) -> dict[str, Any]: | ||
| """Thin wrapper around :func:`predict_single_reaction`.""" | ||
| return predict_single_reaction(self, product_smiles, reactants_smiles) | ||
| return predict_single_reaction( |
There was a problem hiding this comment.
you might want to remove this function in its entirety
There was a problem hiding this comment.
had to add this cuz hallucination_helpers.py calls a predict_single and i have a few doubts on how to change the logic in that file so had to add this.
There was a problem hiding this comment.
Please resolve this.
Darkxzie
changed the title
riya-singh28
left a comment
riya-singh28
left a comment
There was a problem hiding this comment.
@Darkxzie I have left a few comments after my first review.
| >>> checker = build_ml_checker(clf) # doctest: +SKIP | ||
| >>> status, kept = checker("CCO", [["CC", "O"]]) # doctest: +SKIP | ||
| """ | ||
| def _checker(product: str, pathways: list) -> tuple[int, list]: |
There was a problem hiding this comment.
My suggestion is to change this to a simple MLChecker class for now. It resolves issues with the nested functions and provides a clean pathway for future sub-classes, in case multiple types of MLClassifiers are added.
Class MLChecker:
Init(clf):
store classifier
load is_valid_smiles utility
Call(product, pathways):
.
.
If valid_pathways not empty:
return (200, valid_pathways)
Else:
return (400, [])
| f"hallucination_mode='ml' requires a HallucinationClassifier " | ||
| f"or path to saved model — got {type(classifier)}" | ||
| ) | ||
| return build_ml_checker(clf) |
There was a problem hiding this comment.
Please update this according to the suggested new class.
| >>> checker = resolve_hallucination("heuristic", None) # doctest: +SKIP | ||
| >>> callable(checker) # doctest: +SKIP | ||
| True | ||
| >>> checker = resolve_hallucination("ml", "model_out/") # doctest: +SKIP |
There was a problem hiding this comment.
Fix doctest, remove skip, add import statements
riya-singh28
left a comment
riya-singh28
left a comment
There was a problem hiding this comment.
I have requested a few changes.
| ) -> dict[str, Any]: | ||
| """Thin wrapper around :func:`predict_single_reaction`.""" | ||
| return predict_single_reaction(self, product_smiles, reactants_smiles) | ||
| return predict_single_reaction( |
There was a problem hiding this comment.
Please resolve this.
| assert np.all((probabilities >= 0.0) & (probabilities <= 1.0)) | ||
|
|
||
|
|
||
| def test_predict_probability_override_uses_explicit_threshold(): |
There was a problem hiding this comment.
Add docstrings for all the unit tests
There was a problem hiding this comment.
@Darkxzie Please add a test file for hallucination_helpers.py and include tests for the resolve hallucination function with model types = heuristic, ml, and none, among other required tests.
|
@Darkxzie Can you also check why these tests are failing? I have not seen it failing for other PRs yet. |
Hey riya, I checked the failing run. This pr is from a fork branch, so github actions is running without the anthropic api key which is why both test_claud_adv_success and test_call_llm_success are failing. I did this because i currently do not have permission to create branches in this repo. |
3 participants
Description
predict_probability(dataset, threshold=None)so it now returns(labels, probabilities)Fix #(issue)
Type of change
Please check the option that is related to your PR.
Checklist