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

quantiles: Unify probabilities and q #293

Closed
m-mohr opened this issue Oct 26, 2021 · 5 comments · Fixed by #297
Closed

quantiles: Unify probabilities and q #293

m-mohr opened this issue Oct 26, 2021 · 5 comments · Fixed by #297

Comments

@m-mohr
Copy link
Member

m-mohr commented Oct 26, 2021

Originated in #278:

The quantiles process is old and has the issue that one of two optional parameters is required, which we try to avoid nowadays in openEO (then only array_element would be left). Also, the naming is a bit confusing to some communities.

The proposal is to merge probabilities and q into a single parameter q:

{
  "name": "q",
  "description": "Quantiles to calculate. Either a list of probabilities or the number of intervals:\n\n* Provide an array with a list of probabilities to calculate quantiles for. The probabilities must be between 0 and 1 (inclusive).\n*Provide an integer to specify the number of intervals to calculate quantiles for. Calculates q-quantiles with equal-sized intervals.",
  "schema": [
    {
      "title": "List of Probabilities",
      "type": "array",
      "items": {
        "type": "number",
        "minimum": 0,
        "maximum": 1
      }
    },
    {
      "title": "Number of Intervals",
      "type": "integer",
      "minimum": 2
    }
  ]
}

By itself, this is a breaking change. If we want to change this earlier without breaking anything, we could simply change the schema for q as proposed above and deprecate the parameter probabilities. @soxofaan I think I'd like that...

@soxofaan
Copy link
Member

I like this interface better indeed, compared to two optional arguments

@m-mohr
Copy link
Member Author

m-mohr commented Oct 27, 2021

Then let's go for it and fix the mistakes from the past.

@m-mohr m-mohr self-assigned this Oct 27, 2021
@m-mohr m-mohr modified the milestones: future, 1.2.0 Oct 27, 2021
@m-mohr
Copy link
Member Author

m-mohr commented Nov 5, 2021

Hmm, probabilities is listed before q in the parameters. The resulting calls in JS (i.e. non-named parameter languages) would become quantiles(data, undefined, [0,5]) - unfortunate.

@soxofaan
Copy link
Member

soxofaan commented Nov 8, 2021

can't we just update the definition of probabilities and deprecate q?

@m-mohr
Copy link
Member Author

m-mohr commented Nov 8, 2021

Sure, that's also possible. I thought the q was preferred by the Python community due to the xarray implementation so did not consider that. But works for me. I could make a PR for that.

@m-mohr m-mohr linked a pull request Nov 10, 2021 that will close this issue
@m-mohr m-mohr modified the milestones: 1.2.0, future Nov 16, 2021
@m-mohr m-mohr removed their assignment Nov 16, 2021
@m-mohr m-mohr modified the milestones: future, 2.0.0 Mar 8, 2023
m-mohr added a commit that referenced this issue Mar 30, 2023
* quantiles: Deprecate q in favor of probabilities #293

* `quantiles`: Parameter `probabilities` provided as array must be in ascending order.
@m-mohr m-mohr closed this as completed Mar 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants