tests for chaospy.dist.backend.Dist class #31
Conversation
davidovitch
force-pushed
the
development
branch
from
f4febea
to
5b26fea
Compare
|
As I said, this would be much appreciated. I've added a |
davidovitch
force-pushed
the
development
branch
2 times, most recently
from
9503160
to
28a3a62
Compare
Current coverage is 58% (diff: 100%)
@@ development #31 diff @@
=============================================
Files 86 86
Lines 6499 6499
Methods 0 0
Messages 0 0
Branches 0 0
=============================================
+ Hits 3696 3791 +95
+ Misses 2803 2708 -95
Partials 0 0
|
| @@ -223,18 +223,18 @@ def Exponpow(shape=0, scale=1, shift=0): | |||
| return dist | |||
|
|
|||
|
|
|||
| def Exponweibull(a=1, b=1, scale=1, shift=0): | |||
| def Exponweibull(a=1, c=1, scale=1, shift=0): | |||
There was a problem hiding this comment.
Is changing the API like this allowed? This will brake things for Exponweibull users...
There was a problem hiding this comment.
I have no problem with small changes to the API like this.
|
@jonathf, not sure if it is ready to merge, but I would appreciate your opinion on the current state of affairs. Note that I've now changed the name of keyword argument of |
| @@ -396,7 +396,7 @@ def __sub__(self, X): | |||
|
|
|||
| def __rsub__(self, X): | |||
| """Y.__rsub__(X) <==> Y-X""" | |||
| return chaospy.dist.operators.add(X, -self) | |||
| return chaospy.dist.operators.add(self, -X) | |||
There was a problem hiding this comment.
I am not familiar with operator overloading or how to do it correctly, but without these fixes only left subtraction/divition/etc works. This fixes at least the newly added tests in test_dist.py
There was a problem hiding this comment.
This is is the same fix is used for the right addition fix in PR #26
There was a problem hiding this comment.
The reasone I switch the order in #26 was not that it was the right order is that there was that calculation was simplified a bit in the backend. Since addition is symmetric it didn't make a difference.
In the case of __rsub__ this is still the case, but only if the sign follows the right convention. Here the error is on my end since the docstring is incorrect. It should be:
def __rsub__(self, X):
"""X.__rsub__(Y) <==> Y-X"""
In other words, it is what is triggered if you run e.g. 5 - cp.Uniform(0, 1).
So the change would be good if you moved the minus from X to self.
|
Sorry for late response. I will take a look at your PR and come with some feedback this weekend. |
|
|
||
| def __pow__(self, X): | ||
| """Y.__pow__(X) <==> Y**X""" | ||
| return chaospy.dist.operators.pow(self, X) | ||
|
|
||
| def __rpow__(self, X): | ||
| """Y.__pow__(X) <==> X**Y""" | ||
| return chaospy.dist.operators.pow(X, self) | ||
| return chaospy.dist.operators.pow(self, X) |
There was a problem hiding this comment.
Following the text above, these seems incorrect.
The doc in __rpow__ is wrong. Feel free to fix it.
|
|
||
| def __gt__(self, X): | ||
| """Y.__gt__(X) <==> Y>X""" | ||
| return chaospy.dist.operators.trunk(X, self) | ||
| return chaospy.dist.operators.trunk(self, X) |
There was a problem hiding this comment.
Same as above. It should be changed back. Sorry for the incorrect docstrings.
| def _bnd(self, c): | ||
| return 0, self._ppf(1-1e-10, c) | ||
| def _bnd(self, a, c): | ||
| return 0, self._ppf(1-1e-10, a, c) |
|
Thanks for the input! I hope to get another stab at it later this week or during the weekend. |
davidovitch
force-pushed
the
development
branch
from
28a3a62
to
0a30182
Compare
davidovitch
force-pushed
the
development
branch
from
0a30182
to
88e394c
Compare
davidovitch
force-pushed
the
development
branch
from
88e394c
to
613ca41
Compare
davidovitch
force-pushed
the
development
branch
from
19cbd5d
to
3c2ca15
Compare
|
Sorry. I haven't looked at this in a while. I see you have updated the code as I suggested. The only thing worth noting is that you have a few tests commented out. Does this mean you are still working on them, or should they perhaps be removed before I complete the merge? |
|
No problem, I wanted to consider a few more extra test cases before pinging you again, but maybe I could remove the current commented tests, and leave more tests for another PR? Otherwise this might take too long... |
|
Going for more than one PR is fine. |
davidovitch
force-pushed
the
development
branch
from
3c2ca15
to
38b9b65
Compare
davidovitch
changed the title
|
@jonathf ready for merging (I don't think I can change the label to "ready to merge") |
|
Thanks for this. |
I'll start working on some test cases here, but it is still very much a work-in-progress.