Conversation
- Add static method check_fields - Refactor _check_extra_fields, _pre_check_data_sanity using check_fields
…ks for missing, NaN, and invalid response times
…andle warnings for missing data
…eters They are set as default in DataValidator constructor
…e_data fixture directly
…line for setting missing data network
cpaniaguam
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new DataValidator class that encapsulates data validation and preprocessing functionality for HSSM models, and integrates it by having HSSM inherit from DataValidator.
- Introduces DataValidator in src/hssm/data_validator.py with methods to validate and preprocess data.
- Updates HSSM in src/hssm/hssm.py to delegate data handling to DataValidator.
- Adds comprehensive unit tests in tests/test_data_validator.py to ensure robustness.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/test_data_validator.py | Adds unit tests for data validation, though one helper isn’t decorated as a fixture. |
| src/hssm/hssm.py | Integrates DataValidator and removes redundant methods. |
| src/hssm/data_validator.py | Introduces the new DataValidator class with data sanity checks and preprocessing logic. |
…e a common error pattern in test_data_sanity
…ated-module' into 722-reduced-matrix
AlexanderFengler
left a comment
AlexanderFengler
left a comment
There was a problem hiding this comment.
Looks great honestly.
Already polished, didn't see any obvious problem here.
Thanks @cpaniaguam
| # but is expected to exist in subclasses (e.g., HSSM). | ||
| # The 'type: ignore[attr-defined]' comment tells mypy to ignore the missing | ||
| # attribute error here and avoid moving this method to the HSSM class. | ||
| self.model_distribution.extra_fields = [ # type: ignore[attr-defined] |
There was a problem hiding this comment.
I think I resolved cases like this with if hasattr() before. But not sure what the recommended pattern is.
|
@cpaniaguam before merging, could you take 3.12 out of the testing matrix here to make sure all the other stuff runs to completion? We will then bring 3.12 back via another PR, in the context of fixing the dependency situation. |
Remove Python 3.12 from test matrix in GitHub Actions workflow
Codecov ReportAttention: Patch coverage is
🚀 New features to boost your workflow:
|
cpaniaguam
deleted the
722-refactor-data-validation-preprocessing-into-a-dedicated-module
branch
This pull request introduces a new
DataValidatorclass to handle data validation and preprocessing for HSSM models. TheDataValidatorclass centralizes functionality related to data sanity checks, handling missing data, and deadline processing. Additionally, theHSSMclass now inherits fromDataValidator, simplifying its implementation by delegating these responsibilities. Comprehensive tests for theDataValidatorclass have been added to ensure robustness.New
DataValidatorClass:src/hssm/data_validator.py: Introduced theDataValidatorclass with methods for validating data fields, handling missing data and deadlines, and updating extra fields. This class encapsulates data preprocessing logic for HSSM models.Integration with
HSSM:src/hssm/hssm.py:HSSMto inherit fromDataValidator, enabling reuse of data validation and preprocessing methods.HSSMwith calls toDataValidatormethods, such as_handle_missing_data_and_deadlineand_set_missing_data_and_deadline. [1] [2]Test Coverage:
tests/test_data_validator.py: Added extensive unit tests for theDataValidatorclass, covering its constructor, field validation, data sanity checks, handling of missing data and deadlines, and integration with external fields.