Fix indexes 158448605 #106
Conversation
- Support with tests, checked by R package
- An issue has been detected by using exporter tests, which was not reproducible in cr.cube tests alone - Use an existing fixture to reproduce the same error, with the appropriate combination of input parameters to `margin` method
| @@ -1,5 +1,8 @@ | |||
| # History of Changes | |||
|
|
|||
| #### 1.6.9 Bugfix | |||
| - When Categorical Array variable is selected in multitable export, and Scale Means is selected, the cube fails, because it tries to access the non-existing slice (the CA is only _interpreted_ as multiple slices in tabbooks). This fix makes sure that the export cube doesn't fail in such case. | |||
|
|
|||
There was a problem hiding this comment.
I'm sure there's a reason, but this seems unrelated?
There was a problem hiding this comment.
This is just the part that stacks on top of history, that used to live in readme. @percious agreed that we keep 3 latest version changes there, and move the rest to history, with the appropriate link. The version changes in this one, so this shift of the older changes happens, one at a time.
src/cr/cube/cube_slice.py
Outdated
| total = np.sum(base, axis=(axis + 1))[0] | ||
| if axis == 0: | ||
| return base[:, 0] / total[0] | ||
| return base[0, :, 0] / total |
There was a problem hiding this comment.
I'm not saying that we should change this code, but for now this will only work with limited dimensionality cubes, right? So mr x mr or mr x cat x mr or the like aren't useable here, right? We might want a comment noting that for our future selves.
There was a problem hiding this comment.
Gotcha!!! The CubeSlice, where all of this is implemented, will only ever have the max of 2 dims. So, this will work, but we might want to add a test for it. LMK if you have a good one, happy to implement it. We might want to explicitly throw an exception, similar to what you did in R. I can do that if you think it's useful, shall I?
| base = base / np.sum(base) | ||
| return base / np.sum(base, axis=0) | ||
|
|
||
| def index_table(self, axis=None, base=None): |
There was a problem hiding this comment.
I would highly recommend to change base here to baseline. The word base has maaaany other meanings, and this specific kind is not one of them. I can't quite tell if we are exposing this to users, but even if not, baseline or something like it will be less confusing to our future selves here.
There was a problem hiding this comment.
These kind of CR comments totally make my day. Thank you good sir 💯
|
|
||
| This function is deprecated. Use index directly from cube slices. | ||
| ''' | ||
| deprecation_msg = 'Deprecated. Use `cube.slices` instead of `cube`.' |
There was a problem hiding this comment.
Is this the correct deprecation warning?
There was a problem hiding this comment.
Yes, but only as the high level description. I will change it to be more explicit.
| @@ -1067,7 +1073,12 @@ def population_counts(self, population_size, weighted=True, | |||
| ) * population_size * self.population_fraction | |||
|
|
|||
| def index(self, weighted=True, prune=False): | |||
There was a problem hiding this comment.
It would be nice to have a doc-string here (or wherever is appropriate to generate documentation) that says what this is, and that it's deprecated and why it's deprecated. This way users who have gotten used to using this can find the new index_table
| [99.2925720053358, 100.682933745397], | ||
| ]) | ||
| actual = cube.slices[0].index_table(axis=0) | ||
| np.testing.assert_almost_equal(actual, expected) |
There was a problem hiding this comment.
This is suuuuper minor, but could you swap positions with this and the test above it, so you follow the same pattern axis=0 then axis=1 for the tests?
| 92.66856944, 102.10571127, 94.88279498, 84.22596655, | ||
| 92.62629722, 86.77934972, 99.31115914, 98.72846269, | ||
| 99.54678433, 94.13302782, 101.99733805, 102.24392708, | ||
| 97.87112979, 95.08750269, 100.61288629, |
There was a problem hiding this comment.
It would be good to test another row besides just the first row here. Maybe add the last row? or someone in the middle...
|
Closing in favor of #108 |
No description provided.