-
Notifications
You must be signed in to change notification settings - Fork 14
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
Add change from signed int to unsigned int upon copying data #33
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.
The int_to_uint
parameter can lead to two corner cases, please add checks that throw a value error if these occur:
- the input dtype is not a signed integer type
- the
dtype
parameter is passed toCopyVolume
, but it has a different value than the correct unsigned type
Co-authored-by: Constantin Pape <c.pape@gmx.net>
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.
We should also add a test for this, I think the best place is https://github.com/constantinpape/cluster_tools/blob/master/test/downscaling/test_downscaling.py.
Do you want to add signed test data to the archive, or would you prefer generating it? My suggestion would be to use the hdf5 example, simply changing its |
Yes, I agree that sounds like the best solution. |
When I try to run an import it also fails for me.
Is there a way to get a more detailed idea what exactly went wrong? |
It's in the test log:
|
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 looks pretty good now, but still missing:
- a check that makes sure that the
dtype
argument (which is available throughself.dtype
) is compatible with the resulting unsigned dtype ifself.dtype is not None and int_to_uint is True
- a test for
int_to_uint
OK. Runs now. Now to the tests...
As it is now, the Do you like a warning issued if both arguments are given? |
If we assume that the dtype provided by |
No this is more complex than you think (and also than I initially thought). See line 119. If |
Doesn't in the end https://github.com/martinschorb/cluster_tools/blob/c05642dba9ec1ebc7b534c7edb03034ba8917eda/cluster_tools/copy_volume/copy_volume.py#L125 take over everthing before, and hence only |
I overlooked this, but what you did there was not quite correct. You need to use |
I went ahead and fixed the remaining issues (there were a couple. I can just encourage you again to use a linter: it helps tremendously to write correct python code because you will find many issues without needing to actually run the code) and added the test. Also, I think that the code you used to translate between dtypes was wrong:
instead of
see the screenshots below: |
I ll go ahead and merge this. |
Yes, for some reason i assumed that the minimum of |
I don't know if you want to test for this in all potential output formats, or if we simply trust
numpy
on doing it right.