Extract denominator #118
Closed
Extract denominator #118
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
slobodan-ilic
commented
Nov 7, 2018
slobodan-ilic
force-pushed
the
extract-denominator
branch
from
f249b04
to
6c455c4
Compare
slobodan-ilic
force-pushed
the
extract-denominator
branch
2 times, most recently
from
d19e085
to
82041f0
Compare
slobodan-ilic
force-pushed
the
extract-denominator
branch
from
82041f0
to
e79daa6
Compare
* Make `margin` part of API instead of being called with __getattr__ * Make all the arguments explicit * Erradicate revealed bugs in scale means
* Create specialized methods using TDD (unit tests) * Normalize methods * Improve docstrings
* Used to be defined as local method in 2 places * Make it a private static method of the class
* Don't prune univariate MR measures if the denominator is not equal to zero. We used to observe only counts (selected) in such cases. The correct approach is to observe (selected + non-selected).
* Remove convoluted code from margin method * Use newly created _denominator method * Remove xfails
* Put _denominator in the correct alphabetical order
* Add an xfail for the case where proportions are incorrectly pruned * The pruning needs to be based on unweighted bases, and nothing else * In this particular case, proportions are Infs, but they're based on weighted counts, and don't need to be pruned, because the unweighted bases for those proportions are not zero
* Even if weighted margins are 0, don't prune them by default * Only prune if corresponding unweighted margins are 0
* If margin has more than a single dimension, it might need to be pruned in the direction opposite of its calculation (i.e. an MR item might be all-missing) * This represents problems when H&S are inserted (they're not pruned by default), and might introduce non-pruned 0 elements in an otherwise pruned row/col
slobodan-ilic
force-pushed
the
extract-denominator
branch
2 times, most recently
from
ac73c5f
to
25da90d
Compare
* Move exporter tests to crunch-cube * Refactor some of the unnecessary logic that was never hit by either cube or exporter tests
slobodan-ilic
force-pushed
the
extract-denominator
branch
from
25da90d
to
fd0a35a
Compare
|
Closed in favor of #152 |
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 extracts the duplicated code from
proportionsandmargin, and puts it into a separate internal method. Furthermore, it introduces a clear distinction between the concepts of the denominator, versus that one of the margin. There's potentially more refactoring that can be done after this work.Note: Needs rebase on #117 (only last 6 commits relevant).Rebased