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

Change behavior of apply_dimension and reduce #111

Merged
merged 9 commits into from
Jan 16, 2020
Merged

Change behavior of apply_dimension and reduce #111

merged 9 commits into from
Jan 16, 2020

Conversation

m-mohr
Copy link
Member

@m-mohr m-mohr commented Dec 18, 2019

Based on issue #73: Made apply_dimension more usable and made reduce more restricted to real "reduce" use cases.

  • apply_dimension was basically replaced with a completely new definition.
  • reduce: Reducers can't return multiple values any longer. Parameter target_dimension was therefore removed.

What do you think?

@m-mohr m-mohr added the help wanted Extra attention is needed label Dec 18, 2019
@m-mohr m-mohr added this to the v1.0-rc1 milestone Dec 18, 2019
apply.json Outdated Show resolved Hide resolved
apply_dimension.json Outdated Show resolved Hide resolved
apply_dimension.json Outdated Show resolved Hide resolved
reduce.json Outdated Show resolved Hide resolved
@m-mohr
Copy link
Member Author

m-mohr commented Dec 19, 2019

What I'm also thinking about is sorting, for example. It's just the values that change, but also reference system etc should stay intact. What I'm no wondering about is: Should the dimension labels be preserved when sorting? What about preserving the labels when computing the cumsum, for example? This is not really captured any longer with the "enumerated" labels.

@m-mohr m-mohr force-pushed the issue-73 branch 2 times, most recently from 7dd1b4f to 408d2bc Compare December 20, 2019 18:29
@mkadunc
Copy link
Member

mkadunc commented Dec 23, 2019

Re @m-mohr

What I'm also thinking about is sorting, for example. It's just the values that change, but also reference system etc should stay intact. What I'm no wondering about is: Should the dimension labels be preserved when sorting? What about preserving the labels when computing the cumsum, for example? This is not really captured any longer with the "enumerated" labels.

When sorting values, the quantity (variable) remains the same, but the labels change - you go from e.g. time axis time{t_1, t_2, .., t_N} to rank axis rank{1,2,..,N}.

When computing cumsum, the quantity changes (you go from value-at-t_i to sum-value-between-t_0-and-t_i); the labels stay the same, but they (might) change in meaning:

  • before cumsum labels on the time axis are points-in-time when the measurement happended;
  • after cumsum, labels effectively denote the right boundary of the time interval for which the values apply; left boundary of that same interval is the first of the original labels

apply_dimension.json Outdated Show resolved Hide resolved
@m-mohr
Copy link
Member Author

m-mohr commented Jan 6, 2020

When sorting values, the quantity (variable) remains the same, but the labels change - you go from e.g. time axis time{t_1, t_2, .., t_N} to rank axis rank{1,2,..,N}.

Ah, okay. So sort/order don't work well with apply_dimension now:

The dimension labels are preserved when the number of pixel values in the source dimension is equal to the number of values computed by the process.

There's more work needed, also having in mind what @soxofaan wrote in #111 (comment) . Will give it a try and check how this can be streamlined better... (two processes either depending on label changes or cardinality change?)

Co-Authored-By: Miha Kadunc <miha.kadunc@sinergise.com>
@m-mohr
Copy link
Member Author

m-mohr commented Jan 13, 2020

Telco: Agreed, but rename reduce to reduce_dimension.

Edit: I guess, we should also rename filter to filer_labels then?!

@m-mohr m-mohr requested a review from mkadunc January 14, 2020 10:42
@m-mohr
Copy link
Member Author

m-mohr commented Jan 14, 2020

Updated the PR, I think we can merge with one or two reviews.

I also renamed filter to filter_labels as it seems that's what is the direction also with reduce_dimension.

Later, I need to check whether there are references to reduce and filter in API or the docs (e.g. the glossary) and update them.

Remove waiting label from #94 once merged.

@m-mohr m-mohr removed the help wanted Extra attention is needed label Jan 14, 2020
@m-mohr m-mohr mentioned this pull request Jan 14, 2020
@m-mohr
Copy link
Member Author

m-mohr commented Jan 16, 2020

To not further block my work, merging without approvals. If there's still something to change, open a new PR for it.

@m-mohr m-mohr merged commit 1d15463 into draft Jan 16, 2020
@m-mohr m-mohr deleted the issue-73 branch January 16, 2020 09:58
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.

3 participants