Extract measures #117
Merged
Extract measures #117
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
* Move work of resolving type of cube-response and parsing JSON to
a helper method (._cube_dict).
* Change all instances in unit tests of `CrunchCube({})` to
`CrunchCube(None)`. This is a mild case, but shows the main reason to
avoid real work in the constructor, because all unit tests have to
satisfy the arguments required by the real-work.
Get rid of warning-spew that occurred on test runs from calling deprecated CrunchCube.index() in tests.
There are no callers using the optional `adjusted` argument. Having such an argument conflates two separate operations. If adjustment is required, it can easily be performed by the caller on the end-result of as_array().
Remove redundant (copies) integration tests from test_headers_and_subtotals.py
* Remove unused `margin` parameter from CrunchCube.as_array(). If in future such a return value is required it should be provided using a distinct method rather than overloading `.as_array()`. * Remove now unnecessary `margin` parameter from CrunchCube._as_array(). Change call for raw_cube_array to use measure object returned by new `._measure()` method. * Remove now-dead `.raw_cube_array()` method.
* Replace calls to CrunchCube._data() with calls to ._counts() or ._measure() depending on context, using the .raw_cube_array property of the returned measure object to access the appropriate array formerly provided by ._data(). * Remove CrunchCube._data() and its helpers `._flat_values()` and `._shape` as they are now dead code.
* Add `make unit-coverage` task to Makefile that shows our unit test coverage is pretty spotty, indicating an over-reliance on integration tests (which do not test fine-grained behaviors).
scanny
force-pushed
the
extract-measures
branch
from
392005a
to
6e10548
Compare
slobodan-ilic
requested changes
Nov 7, 2018
| unweighted counts are returned. | ||
| """ | ||
| return ( | ||
| self._measures.means if self._measures.means is not None else |
There was a problem hiding this comment.
Do we require is not None part here? It seems that the return value is either None or an instance, in which case the if self._measures.means would be enough.
There was a problem hiding this comment.
Maybe the explicit form would be more readable here:
measures = self._measures
if measures.means is not None:
return measures.means
if weighted:
return measures.weighted_counts
return measures.unweighted_counts
I believe it would also not hide potential misses by coverage (not sure about this though). I tried it locally, and it increases the unit test coverage from 65.94% to 66.10%
| else json.loads(cube_response) | ||
| ) | ||
| # ---cube is 'value' item in a shoji response--- | ||
| return cube_dict.get('value', cube_dict) |
There was a problem hiding this comment.
The case when there is a "value" key doesn't seem to be tested by unit tests.
| @@ -1466,6 +1466,13 @@ def means(self): | |||
| return None | |||
| return _MeanMeasure(self._cube_dict, self._all_dimensions) | |||
|
|
|||
| @lazyproperty | |||
| def missing_count(self): | |||
| """numeric representing count of missing rows in cube response.""" | |||
There was a problem hiding this comment.
I'm not sure that "row" is the thing that's being counted as missing here...
| return ( | ||
| self._measures.weighted_counts if weighted else | ||
| self._measures.unweighted_counts | ||
| ) |
There was a problem hiding this comment.
Writing it like this:
if weighted:
return self._measures.weighted_counts
return self._measures.unweighted_counts
Increases the unit test coverage from 65.94 to 66.01 percent.
| Returns | ||
| res (ndarray): Tabular representation of crunch cube | ||
| """ | ||
| return self._apply_missings_and_insertions( | ||
| self._raw_cube_array(weighted, margin), | ||
| self._measure(weighted).raw_cube_array, |
| @@ -1,22 +0,0 @@ | |||
| import os | |||
There was a problem hiding this comment.
I'm not sure that this particular commit corresponds to the message.
Closed
slobodan-ilic
approved these changes
Nov 7, 2018
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 refactoring extracts a
_Measuresobject and a hierarchy of measure objects that subclass_BaseMeasure. Specifically, these are_MeanMeasure,_UnweightedCountMeasure, and_WeightedCountMeasure.These objects extract the measure-related logic out of CrunchCube as pure-functions (lazyproperties). The new objects have 100% unit test coverage and simplify CrunchCube.