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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

draft: Joseba/multivariate quantile #33

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

jdalch
Copy link
Collaborator

@jdalch jdalch commented Oct 5, 2023

No description provided.

@jdalch jdalch linked an issue Oct 5, 2023 that may be closed by this pull request
@M-Mouhcine M-Mouhcine self-requested a review October 10, 2023 16:11
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Print traces should be remove. I suggest to replace with a logger from logging package.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work ! Few remarks:

  1. PEP8 recommends a line length of 79 characters. You should set your black formatter to that convention to avoid unneeded reformatting. Check this link.
  2. Type hints should be consistent with older versions of python. Use Union[float, np.ndarray] instead or float or np.ndarray.
  3. What is this condition axis % a.ndim == -1 supposed to check ?
  4. Initializing axis to 0 or None seems to have no effect on the quantile computation, which is inconsistent with the docstring. Here is an example:
a = np.array([[1, 2, 3, 4, 5], [10, 20, 30, 40, 50], [100, 200, 300, 400, 500]])
q = np.array([0.1, 0.1, .5, .1, 0.9])
print(quantile_unweighted(a, q, axis=None)) # same result as quantile_unweighted(a, q, axis=0)
  1. Following up on the last question, is there a case where axis > 0 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello,

  1. I can't find the option in viscose that your link says.
  2. Ok.
  3. As per the error message below, it checks that you are not trying to compute the quantile along the feature axis.
  4. I changed the docstring. This is the expected behavior. For higher dimensional a, the behavior will be different if axis=None or if axis=0.
  5. Yes, check line 534 of calibration.py.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About point 3, if I understand correctly:

The condition axis % a.ndim == -1 is not correct to check if the user selected the feature axis. In Python, the modulo will always be positive because a.ndim is.

If you want to use modulo, you need to add a negative sign in front of a.ndim. This should work as intended: axis % -a.ndim == -1. However, I would rather pick a more expressive condition such as (a.ndim - axis == 1) or (axis == -1) or axis not in (a.ndim-1, -1).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I corrected this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be removed.

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.

Multivariate quantile
2 participants