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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Patch for issue #692 #697

Merged
merged 6 commits into from
Mar 3, 2020
Merged

Conversation

anindex
Copy link
Contributor

@anindex anindex commented Mar 2, 2020

This PR proposes bug fix for issue #692 by adding explicitly SizeType and IndexType into template arguments when initializing patttern_t in StoreHDF read and write.

@devreal
Copy link
Member

devreal commented Mar 2, 2020

@anindex Do I understand your commits right that HDF5 is not enabled in CI? It would be nice to have a test case for reported issue somewhere. I believe there is a dedicated set of HDF5 tests.

@anindex
Copy link
Contributor Author

anindex commented Mar 2, 2020

I will write a test case for HDF5 read and write in other PR, for now, I will keep this fix minimal :)

@anindex anindex changed the title Patch for issue #692 and (possibly) adding test cases for Patterns Patch for issue #692 Mar 2, 2020
@devreal
Copy link
Member

devreal commented Mar 3, 2020

Looking at the patch again I realized that it fixes the symptoms of the problem (a type-mismatch in the HDF5 driver) but eventually we should address the underlying issues of the DASH types being inflexible with regard to the types they accept. TilePattern should, for example, have a c'tor templated on the type of SizeSpec and convert it if necessary. Maybe that will come as a byproduct of the pattern test suite :)

@devreal
Copy link
Member

devreal commented Mar 3, 2020

Also: the codecov CI seems broken. Merging anyway.

@devreal devreal merged commit f84e89e into dash-project:development Mar 3, 2020
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.

2 participants