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

quantile across dimension #3002

Merged
merged 25 commits into from
Aug 23, 2023
Merged

quantile across dimension #3002

merged 25 commits into from
Aug 23, 2023

Conversation

dvd101x
Copy link
Collaborator

@dvd101x dvd101x commented Jul 22, 2023

Hi, while working on #1552 I think this is the minimum changes to make it work, but maybe there are some topics to discuss.

  1. By researching quantileSeq, I've seen some other references and I think it could be changed to just quantile
  2. A different method could be implemented for example linear (numpy's default)
  3. It could be refactored to use more of typed capabilities
  4. utils/lastDimToZeroBased doesn't apply directly for this function, in my opinion is too specific.
  5. Other stat functions are also missing dimensions functionality: mad, median, mode and prod
  6. It seems like other stat functions could benefit from apply to reduce the amount of code

Please let me know if any of these topics could be addresses in this PR.

@josdejong
Copy link
Owner

The PR looks good and well tested, thanks David. I think we can merge it as-is.

About your discussion points:

  1. Renaming quantileSeq to quantile: I don't remember why the function was originally named like this. I would prefer not "just" renaming the function, that is a breaking change again. But if there is a good reason for it we can consider it. Is the name quantile more common in other applications for example?
  2. I'm not sure what you mean with A different method could be implemented for example linear (numpy's default)
  3. Would you like to change lastDimToZeroBased to make it more usable in different cases? Do you have a proposal?
  4. The function quantileSeq can indeed use some refactoring if you want to give that a go. A PR would be welcome, but let's please create a separate PR for this.
  5. Yes I would love to add the dimension argument to other functions too. (let's do that in a separate PR :))
  6. Definitely! You're full of ideas to improve the code base :). A separate PR with a refactor would be nice.

@dvd101x
Copy link
Collaborator Author

dvd101x commented Jul 27, 2023

Thank you, Jos,

Ok, that sounds good to have those topics on a different PRs. I think I left some unwanted modifications in the docs which I will fix soon.

  1. After further review, I noticed quantileSeq has a parameter that is not common for other quantile, If probOrN <= 1 then it is prob as other quantile but if probOrN is an integer > 1 then it acts differently by splitting the range from 0 to 1 in N places and returning an array of prob and the rest works like other quantile. Here are some references of other quantile that are similar between them:
  1. In numpy there are about 9 different methods to compute quantile. The linear method instead of giving the average of two values when it falls in between, does a linear interpolation.
  2. Currently lastDimToZeroBased is limited to where the number of arguments is exactly two. quantileSeq has more than two arguments so it can't use it as such.
  3. Ok, I will try
  4. Ok, thanks!
  5. Ok, thanks! :D

I made an unwanted modification
@josdejong
Copy link
Owner

Thanks for the reply and all your inputs of this week and last week on the project David, I'll look into them after the holidays, sorry for the delay.

@dvd101x
Copy link
Collaborator Author

dvd101x commented Aug 5, 2023

Hi Jos, thanks for the reply

Please don't worry about it, and have great holidays!

@josdejong josdejong merged commit 3ab9bc1 into josdejong:develop Aug 23, 2023
7 checks passed
@josdejong josdejong mentioned this pull request Aug 23, 2023
@josdejong
Copy link
Owner

Published now in v11.10.0

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.

2 participants