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

Improve summary of order process #424

Merged
merged 2 commits into from
Mar 31, 2023
Merged

Conversation

soxofaan
Copy link
Member

spin-off from #409 😛

@m-mohr
Copy link
Member

m-mohr commented Mar 15, 2023

A change here should probably be aligned with rearrange.

For me in this proposal it's not clear what the order is. This sounds a bit like whether it would return "ascending" or "descending" or "unsorted" and not the indices for the array in a sorted order ;-)

@soxofaan
Copy link
Member Author

soxofaan commented Mar 16, 2023

A change here should probably be aligned with rearrange.

Because rearrange uses "permutation" in its summary? I think that's fine.

For me in this proposal it's not clear what the order is. This sounds a bit like whether it would return "ascending" or "descending" or "unsorted"

Ok fair. An alternative, stealing from numpy argsort:

Get indices that would sort the array

FYI: what I don't like about the current summary "Create a permutation":

  • "create a" sound like the process just randomly generates some permutation, regardless of the original data
  • "permutation": it's not always obvious if "permutation" refers to the set of indices to implement the rearrangement, or if it is about the result after rearranging

so "create a permutation" roughly sounds like "randomly shuffle the array" to me

@m-mohr
Copy link
Member

m-mohr commented Mar 30, 2023

@soxofaan I made a couple of changes to the PR, especially to link between the processes. Can you check whether this works for you?

@soxofaan
Copy link
Member Author

yes it's better now

apparently I can approve this PR, I guess because I started it myself

@m-mohr m-mohr merged commit 4dff343 into Open-EO:draft Mar 31, 2023
@m-mohr
Copy link
Member

m-mohr commented Mar 31, 2023

True, I've just merged it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants