Removed constructor from DataValidatorMixin#905
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes the constructor from DataValidatorMixin (treating it as a true mixin whose state is provided by HSSM) and updates the test suite to instantiate a concrete helper class instead.
Changes:
- Removed
DataValidatorMixin.__init__and replaced it with class-level attribute annotations. - Updated
tests/test_data_validator.pyto use a concreteDataValidatorTestersubclass for testing. - Added (currently commented-out) scaffolding for handling choice-only models in
_handle_missing_data_and_deadline.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/hssm/data_validator.py |
Removes mixin constructor and adds attribute annotations; adds commented-out choice-only handling notes. |
tests/test_data_validator.py |
Introduces DataValidatorTester to allow constructing a test instance now that the mixin has no constructor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…nly-models Added `is_choice_only` field for `Config` class and updated tests
AlexanderFengler
left a comment
AlexanderFengler
left a comment
There was a problem hiding this comment.
Small comments, but basically ready? Main confusion with all the open PRs that I need to resolve on my end is why is actually meant to be independently mergable. I reviewed this one, presupposing it is meant as a standalone PR. @digicosmos86
| def _handle_missing_data_and_deadline(self): | ||
| """Handle missing data and deadline.""" | ||
| if not self.missing_data and not self.deadline: | ||
| # In the case of choice only model, we don't need to do anything with the |
There was a problem hiding this comment.
probably can't merge this way?
What is needed here, slight test rewrite?
There was a problem hiding this comment.
There are some new rules about to be introduced to the HSSM class. Basically, since there is no rt fields in choice-only models, some of these checks are either no longer needed or can be largely skipped. Another PR will be added to address this. The merge conflicts can get really crazy since we have so many outstanding PRs. I will only introduce the new PR after we can merge the current ones
| extra_fields: list | ||
|
|
||
| dv.model_distribution = DummyModelDist() | ||
| dv.model_distribution = DummyModelDist() # type: ignore[assignment] |
There was a problem hiding this comment.
Why not something like:
dv.model_distribution = cast(ExpectedModelDistributionType, DummyModelDist())
There was a problem hiding this comment.
cast is really weird too and is only done to please the type checker. it's a bit redundant in tests
There was a problem hiding this comment.
After checking, I don't think this is a change worth implementing. You need to explicitly declare a type derived from pm.Distribution that has extra_field and other attributes specifically for this test. As a unit test this is a bit of an overkill
|
@digicosmos86 this one also seems like 99% stage. |
AlexanderFengler
left a comment
AlexanderFengler
left a comment
There was a problem hiding this comment.
ok for me to merge. Thanks @digicosmos86
Note: I am trying to separate #903 into several smaller PRs so it's easier to review and track the changes
This PR removes the constructor from DataValidatorMixin. As a mixin, its internal states should come from the HSSM class. Currently, its constructor is called from within HSSM class with default values, and then static methods are used, which can lead to inconsistencies.
This PR only fixes this class. Will need to modify HSSM class itself in future PRs