Document/Unit Test gempyor.statistics
#304
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Wrote draft documentation for the `statistics` module in Google style guide.
* Added missing type annotations and corrected already existing ones. * Applied black formatter to the file, including manually correcting some line-length issues. * Rearranged dunder methods.
…docs Merging the 'unit-test-gempyor-parameters' branch into 'GH-300/statistics-unit-tests-docs' to get the confuse related testing utilities.
* Created initial unit testing infrastructure for the `Statistic` class from `gempyor.statistics`, starting with invalid regularization name value error. * Added default to `getattr` call to make unsupported regularization value error reachable. Should obsolete with better documentation.
* Added a test fixture to test the attributes of the `Statistic` class. * Removed unnecessary `tmp_path` pytest fixture dependency. * Improved documentation on the `Statistic` class' attributes and added a raises section for the constructor.
Added a test fixture for the result of calling `str` and `repr` on an instance of the `Statistic` class.
The return of `Statistic.llik` is actually an xarray DataArray instead of a float, but summed along the date dimension.
In particular see #300 (comment).
Added the initial unit tests for the regularization methods, `_forecast_regularize` and `_allsubpop_regularize`, of the `Statistic` class. The tests are general and do not make claims about correctness for now.
Added unit tests for the `apply_resample` method, including creating a new factory, `simple_valid_resample_factory`, which hits the "resample config present" of the code path.
Added unit tests for `apply_scale` method including a new factory that produces an input set with a 'scale' config. Fixed a bug where the scale function was not applied even if provided. This is a *breaking* change, but doesn't affect currently existing test suite, need to see if this affects any currently existing config files.
Added unit tests for the `apply_transforms` method of `Statistic`, including making a new factory that includes both resampling and scaling configuration.
Created global `all_valid_factories` that can be passed directly to the `pytest.mark.parametrize` decorator to test methods of the `Statistic` class against many configurations.
Added unit tests for the `llik` method of the `Statistic` class. Had to change factories to use RMSE by default for likelihood distribution since the poisson distribution only has integer support.
Was previously using `xarray.DataArray` for `model_data` and `gt_data` in unit testing the `Statistic` class since that is what many methods expect. It seems though the main entry to the class, `compute_logloss` takes an `xarray.DataSet` that the class splices into `xarray.DataArray`s. The unit tests now more accurately reflect this.
* Created initial unit tests on the `compute_logloss` method of `Statistic`, checking for structure but not correctness. * Updated documentation for `compute_logloss` to reflect the possible `ValueError` and the correct input types expected. * Changed internal variable of that method to a float to get a consistent float return for the second tuple entry from `compute_logloss`.
Added a test fixture that confirms the `ValueError` raised when model data and ground truth data do not have the same shapes in `Statistic.compute_logloss`.
There were entries in the mock configs, modeled on existing configs, that are not considered by the `Statistic` class at all. Removed for clarity.
emprzy
previously approved these changes
Aug 23, 2024
TimothyWillard
dismissed
emprzy’s stale review
The merge-base changed after approval.
|
I think this got lost at some point after GH-277 was merged in, but this should be ready to review. I will address these issues from GH-300 in a follow-up PR after this one (want to establish a baseline of testing for the class first):
likelihood = xr.DataArray(likelihood, coords=gt_data.coords, dims=gt_data.dims)
# TODO: check the order of the arguments
return likelihood
and
|
jcblemai
approved these changes
Oct 11, 2024
Would you mind referencing that commit on the main issue so that fact doesn't get lost? Thanks! |
isn't it already cause it contains the issue number ? I'll add a comment in case |
Ah, you're right, I missed that. |
TimothyWillard
added
bug
saraloo
approved these changes
Oct 18, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR:
gempyor.statisticsmodule and its only classStatistic,Statisticclass,gempyor.statisticsusing the__all__dunder, andThis PR does not:
In the process of this PR come across some issues that I've noted on the main issue, GH-300.
This PR derives from the branch in GH-277 because I needed the confuse related testing utilities, so it would be much easier to review this PR after reviewing/merging GH-277.