-
Notifications
You must be signed in to change notification settings - Fork 55
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
Features/305 cov #322
Features/305 cov #322
Conversation
…le multiple matrices
known corner case issues: - empty nodes - node with a single datapoint (matmul)
added tests for raises as well as correctness, all of the background cases should be covered by the functions which cov relys upon
Codecov Report
@@ Coverage Diff @@
## master #322 +/- ##
==========================================
+ Coverage 97.27% 97.34% +0.07%
==========================================
Files 53 53
Lines 10221 10395 +174
==========================================
+ Hits 9942 10119 +177
+ Misses 279 276 -3
Continue to review full report at Codecov.
|
edge case for matmul was in the event that the block size in any dimension as 1 this would result in not finding the remainders, a condition in the block setters was added to fix this the N-D case was removed from dot() because this is not supported by the ht.__mul__ function, nor was the torch.dot capable of this functionality
@@ -210,8 +210,6 @@ def array(obj, dtype=None, copy=True, ndmin=0, split=None, is_split=None, device | |||
if bool(copy): | |||
if isinstance(obj, torch.Tensor): | |||
obj = obj.clone().detach() | |||
elif isinstance(obj, np.ndarray): |
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.
Why is this disallowed here?
heat/core/tests/test_statistics.py
Outdated
actual = ht.array([[1, -1], [-1, 1]], split=0) | ||
self.assertTrue(ht.equal(cov, actual)) | ||
|
||
f = h5py.File('heat/datasets/data/iris.h5', 'r') |
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 is a bad idea. h5py is an optional feature, it might not necessarily be installed. We should fall back here to a iris.csv version that we should also have in the datasets.
it might as well be useful for testing purposes to use ht.load_csv instead
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.
i found a bug in load_csv. i have made an issue (#368) at the moment this problem does not allow for the the usage of load_csv.
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.
Yes, I have assigned Simon already. So we need to wait here for a proper load_csv()
heat/core/tests/test_statistics.py
Outdated
|
||
np_cov = np.cov(data, data, rowvar=True) | ||
|
||
htdata = ht.load('heat/datasets/data/iris.h5', 'data', split=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.
see h5py thing
closes #305 by implementing cov in the same style as numpy
closes #375 by implementing dot