-
Notifications
You must be signed in to change notification settings - Fork 15
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
draft: Joseba/multivariate quantile #33
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work ! Few remarks:
- PEP8 recommends a line length of 79 characters. You should set your black formatter to that convention to avoid unneeded reformatting. Check this link.
- Type hints should be consistent with older versions of python. Use
Union[float, np.ndarray]
instead orfloat or np.ndarray
. - What is this condition
axis % a.ndim == -1
supposed to check ? - 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)
- Following up on the last question, is there a case where
axis > 0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello,
- I can't find the option in viscose that your link says.
- Ok.
- As per the error message below, it checks that you are not trying to compute the quantile along the feature axis.
- 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.
- Yes, check line 534 of calibration.py.
There was a problem hiding this comment.
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)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I corrected this.
dirty_nbs/quantile.ipynb
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be removed.
No description provided.