Document/Unit Test gempyor.parameters
#277
Conversation
Wrote, documented, and unit tested `create_confuse_subview_from_dict` and `create_confuse_rootview_from_dict` functions to easily create confuse package objects from dicts for use in parametrized unit tests.
Initialized the unit tests for the `gempyor.parameters.Parameters` class with fixtures for the exceptions raised by the class' constructor. Left a note about the inability to reach one of the `ValueError`s, will have to revisit, either with a way to reach that exception or refactor the dead code.
Wrote a unit test fixture for correctly instantiating the `Parameters` class as well as checking its documented attributes. Fixed typo in attributes documentation.
Added `partials_are_similar` to `gempyor.testing` for testing if `functools.partial` objects are similar enough for unit testing purposes along with corresponding documentation and unit tests.
Switch from manually testing partials to using `partials_are_similar` in the `gempyor.parameters.Paramters` unit tests.
Added test fixtures for the `picklable_lamda_alpha/sigma` methods of `gempyor.parameters.Parameters`. Also added some light inline comments for the unit tests.
Added a test fixture for the `get_pnames2pindex` method of `gempyor.parameters.Parameters`.
Added a test fixture for `parameters_quick_draw` including coverage for the shape issues with time series parameters. Added a note to the documentation for `Parameters.parameters_quick_draw` about time series parameters.
Added a test fixture for `Parameters.parameters_load` as well as add some to the fixture for `parameters_quick_draw`. Light documentation updates to the corresponding method to clarify conforming sizes with time series parameters.
Added a test fixutre for the `getParametersDF` method of `gempyor.parameters.Parameters`.
Did not implement a test fixture for the `parameters_reduce` method of `gempyor.parameters.Parameters` for now. Left a note explaining the blocker of getting a handle on the `NPI` module.
TimothyWillard
added
documentation
There was a problem hiding this comment.
Seems like a reasonable start. The tests need some DRYing and elimination of magic constants.
Seems worthwhile to tail off a few issues for refactoring as well - e.g. extracting sub-parser methods / object definitions for timeseries and distros; pruning no-longer-used methods
| corresponds to the `npar` attribute of this class. | ||
|
|
||
| Note: | ||
| If any of the parameters are 'timeseries' type parameters then `n_days` and |
There was a problem hiding this comment.
must they? doesn't look like any errors raised if not?
There was a problem hiding this comment.
Yes, you get an exception raised by numpy about shapes not being compatible on this line:
param_arr[idx] = self.pdata[pn]["ts"].valuesI suppose you could provide a single date time series and it would be fine since numpy could make those shapes compatible (although, the constructor doesn't allow missing dates for time series params). I decided to place this info under the Notes (I need to fix typo) section instead of the Raises since this method doesn't raise the exception itself.
Parameterized `test_timeseries_parameter_has_insufficient_dates_value_error` test fixture by moving custom DataFrame to shared `MockData` class.
Export the `partials_are_similar` function from the `gempyor.testing` module using the `__all__` dunder.
Added a function with the same core logic as `gempyor.utils.as_random_distribution` that is decoupled from the `confuse.ConfigView` class along with corresponding documentation and tests.
Decoupled the `test_parameters_instance_attributes` test fixture from a specific set of inputs and generalized it to accept arbitrary factories that generate valid input sets for `Parameters`.
Parametrized the `test_picklable_lamda_alpha`, `test_picklable_lamda_sigma`, and `test_get_pnames2pindex` test fixtures to accept arbitrary valid inputs.
|
No need to review yet, still WIP per the suggestions above. |
Added a function to `gempyor.testing` to determine if a given value is supported by a given distribution and its paramters. It does not test if said sample is plausible or not.
Parametrized the test fixture for the `Parameters.parameters_quick_draw` method. Also developed a generic `MockParametersInput` class that can be used to easily facilitate the parameterization of `gempyor.parameters.Parameters` unit tests.
flepimop/gempyor_pkg/tests/testing/test_create_confuse_rootview_from_dict.py
Outdated
Show resolved
Hide resolved
Combined `create_confuse_rootview_from_dict` and `create_confuse_subview_from_dict` into one `create_confuse_configview_from_dict` function in `gempyor.testing`.
Updated unit tests for `gempyor.parameters.Parameters` to use the new `create_confuse_configview_from_dict` helper instead of the two prior versions of this function.
|
@saraloo, @emprzy, or @pearsonca is there any chance we could get this review and merged soon? |
Resolved conflicts in the `gempyor.utils` module related to the `typing` module imports.
Added a brief description to the example for the `create_confuse_configview_from_dict` function that also shows the corresponding yaml that is represented by the example.
TimothyWillard
dismissed stale reviews from saraloo, emprzy, and jcblemai
via
5b4d062
There was a problem hiding this comment.
@TimothyWillard checking my understanding here: this file effectively establishes a module within gempyor, corresponding to testing support functions, yes?
if so, what are the benefits of making those functions part of the public interface of gempyor? i'm familiar with the downsides, and am more accustomed to seeing this sort of thing as internal to the test suite, not exported. But I can imagine possible benefits?
There was a problem hiding this comment.
Yeah, that understanding is correct.
The unit tests are defined outside of the package so they need to be able to import these functions from gempyor. I could set __all__ = [] anyways to exclude these from the "public interface", there are ways to easily get around the __all__ restrictions. Other options would be either 1) moving these functions into an appropriate place in the tests folder or 2) moving the tests into gempyor.
There was a problem hiding this comment.
got it - this should be a next-issue-problem and seems like the right answer is 2).
if we're thinking of gempyor as its own little capability / package, it should have its own internal tests.
Document/Unit Test `gempyor.parameters`
This PR:
gempyor.parametersmodule and its only classParameters,Parametersclass,gempyor.parametersusing the__all__dunder`.This PR does not:
list/dicttypes not being fully specified),parameters_reducemethod of theParametersclass.This is the first in one or two more PRs to address GH-276. I didn't implement any tests for
parameters_reduceyet because I still need to get a handle on theNPIobjects. However, I think it would be best to get this first pass done to set ground for the next pass.