-
Notifications
You must be signed in to change notification settings - Fork 160
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
Improve testing infrastructure. #871
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #871 +/- ##
==========================================
+ Coverage 84.97% 85.62% +0.65%
==========================================
Files 71 73 +2
Lines 5118 5461 +343
==========================================
+ Hits 4349 4676 +327
- Misses 769 785 +16 ☔ View full report in Codecov by Sentry. |
8e44c75
to
1a87fe7
Compare
687d71d
to
9a97a69
Compare
Adds/reimplements abstractions for the following: * Create multi-dimensional array filled with suitable values. * Traits for accessing values. * Traits for hiding the difference of DataSet and Attribute. * Useful utilities such as `ravel`, `unravel` and `flat_size`.
9a97a69
to
586310f
Compare
`DataGenerator`. For example: | ||
``` | ||
auto dims = std::vector<size_t>{4, 2}; | ||
auto values = testing::DataGenerator<std::vector<std::array<double, 2>>::create(dims); |
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 would use a library for this! For example, you can use the NumPy equivalent xtensor. Writing and maintaining some extra custom code is extra work and error-prone. I would say that testable data are as simple as
xt::xarray<int> a = xt::arange<int>(5 * 4 * 3).reshape({5, 4, 3});
Then you can use functions like ravel
, ravel_index
, ... just like in NumPy. No new API, no new code.
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'd like to test the following:
template<class Container>
void check_write_dataset(const Container& container) {
file.createDataSet("foo", container);
}
Hence, I need to able to create arrays of all supported types
std::vector<T>
std::vector<std::array<T, 4>>
std::array<std::vector<T>, 4>
boost::
// xt:: (once supported in core)
and so forth, and be able to fill them with exactly the correct values. Being able to have any container filled with sensible values, requires being able to a) allocate containers of a given shape, b) assign values. Those two feature make up a good chunk of the PR. Then there very minor niceties like copying, etc.
The second issue is that I need a somewhat uniform API for creating the product of template<class T> Container
with a bunch of different scalar types T
. That's the other large chunk of the PR.
How does an xt::array
help in this situation? Yes, I can test XTensor, but I can't test writing of any of the other containers. If I use XTensor arrays to write, I can read the other containers, but I still can't check that the values are correct, because I don't have access to some API unifying trait to hide the difference of a[i][j]
, a(i,j)
etc. (gets worse in 3D).
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.
If there is a pressing reason to use a custom implementation, of course, it's fine. Anyway, this is not a public implementation, nor will change a lot.
I just thought it would be easier to write some casters but avoid implementing (un)ravel(index) functions and equality operators.
``` | ||
using trait = testing::ContainerTraits<std::vector<std::array<int, 2>>; | ||
// auto x = values[1][0]; | ||
auto x = trait::get(values, {1, 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.
Following the command above:
auto x = a(1, 0);
there is also
auto y = a.flat(10)
to read directly from the flat storage.
(comment applicable below)
It's a good idea to streamline testing. However, as I stated in some comments, there are existing libraries for this, and I would strongly suggest not to seek a custom implementation. Using a library comes with the advantage that:
|
In defense of the complexity: it found several bugs/quirks or things to follow up on:
It would trivially fix: Any non-uniformity is very painful to us (not just our users), and therefore has a higher chance of getting noticed and fixed. The following come to mind: |
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 think it's a nice idea to unify Dataset and Property testing. You also added tests convering that, so I really don't have anything to nitpick about. If you want to wait for other reviews, great, but from my side good to go
Adds/reimplements abstractions for the following:
ravel
,unravel
andflat_size
.