Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decouple pruning and missingness #147

Merged
merged 2 commits into from
Apr 2, 2019

Conversation

slobodan-ilic
Copy link
Contributor

@slobodan-ilic slobodan-ilic commented Apr 2, 2019

Break ugly method that does 2 things that shouldn't be together into two independent methods.

* Break method for missingness and H&S into two methods
* Unit tests still need to be fixed
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 73f6914 on decouple-pruning-and-missingness into 25adbdd on master.

@slobodan-ilic
Copy link
Contributor Author

So, @scanny I've modified how we interpret valid indices. However, I've had to add another parameter to the __init__ of some classes in dimension.py, to be able to fetch the index of an element in only valid elements (and not all elements). Please see if you can suggest something better there.

However, I believe it's a step forward in the cube code, where some decoupling is done. In the future, I plan to do more of this, but from cube_slice.py mostly.

@scanny
Copy link
Contributor

scanny commented Apr 2, 2019

I'm not sure I can propose a better way to do that; it's been a while since I've had my head into this code so I can't remember what gets done before other things. But it looks fine to me.

I think the big payoff comes when we get a FrozenCube (or whatever we name it), because that will allow the statefulness to come out and greatly reduce the amount of parameter passing that makes things so hard to reason about. The state is really insidious. Like you have to reason about "okay, is this table I have before or after applying missing? Does it have headings and subtotals yet? Oh shoot, I need the version before that. etc."

Once you have CubeSlice pretty well cleaned up I'd be inclined to create a FrozenSlice that just wraps the functionality of CubeSlice. That would be pretty fast because it's just mapping CubeSlice calls using instance variables. Initially that would allow us to change the interface of exporter without really changing any functionality. But then, you could incrementally reimplement the lazyproperties until the FrozenSlice stood on its own, and then we could use that base for new development after that.

I guess the short version of my take is you aren't really going to be able to get clean until you get rid of the state (like an ndarray in various stages of mutation). We can talk more about this when we get a chance to connect real-time.

@slobodan-ilic slobodan-ilic merged commit 34eda08 into master Apr 2, 2019
@slobodan-ilic slobodan-ilic deleted the decouple-pruning-and-missingness branch May 17, 2019 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants