Add support for float16 datatypes #179
Conversation
byrnHDF
added
Type - Improvement
src/org.hdfgroup.hdfview/hdf/view/dialog/NewCompoundAttributeDialog.java
Show resolved
Hide resolved
src/org.hdfgroup.hdfview/hdf/view/dialog/NewCompoundAttributeDialog.java
Show resolved
Hide resolved
src/org.hdfgroup.hdfview/hdf/view/dialog/NewStringAttributeDialog.java
Outdated
Show resolved
Hide resolved
src/org.hdfgroup.hdfview/hdf/view/dialog/NewStringAttributeDialog.java
Outdated
Show resolved
Hide resolved
| { | ||
| Object theValue = newValue; | ||
| if (isFLT16) | ||
| theValue = Short.toString(Float.floatToFloat16(Float.parseFloat((String)newValue))); |
There was a problem hiding this comment.
Why is this set as a string here rather than a real data value?
There was a problem hiding this comment.
Might be an artifact before the Display/Converter changes?
There was a problem hiding this comment.
Could be. I never created one, but it'd be useful to have a test file with a float16 member inside a compound, since this seems like editing one of those values would convert it to a String and either fail on write or cause weird issues later during read.
There was a problem hiding this comment.
I did test a change value and it worked - will need to check into container types.
There was a problem hiding this comment.
You can see the other setDataValue calls below return Float.float16ToFloat((short)bufObject) rather than a string. This almost certainly doesn't need the Short.toString( part and I'm surprised it doesn't cause problems.
There was a problem hiding this comment.
Maybe - I know not having it failed the first time I tried - will try again since changing other code.
There was a problem hiding this comment.
Needs to be a string - fails without it
There was a problem hiding this comment.
Then something is wrong elsewhere. For numerical values the DataProvider should be setting actual values and not strings. The only time numerical data should be working on a String is when converting to displayable formats in the DataDisplayConverters.
There was a problem hiding this comment.
The trouble is that float16 is a Short underneath and everything was triggering on that type.
There was a problem hiding this comment.
Right, I understand, but it shouldn't be a string. It's unfortunate that the implementation chose to encode the value in a short, because it forces us to implement "wrapper" setDataValue functions just to check if a value is a float16 value so that we can convert it from a short to a real float type, but I suppose it is what it is. But, the data array we work with should always be in terms of either short or float and then converted to String in the DataDisplayConverters and float elsewhere where we need a real value (if we didn't already choose to convert it to a float for the data array).
src/org.hdfgroup.hdfview/hdf/view/TableView/DataDisplayConverterFactory.java
Outdated
Show resolved
Hide resolved
| else | ||
| strValue = Float.toString(Float.float16ToFloat((short)value)); | ||
|
|
||
| if (count > 0 && strValue.length() > count) |
There was a problem hiding this comment.
Not sure what this line is trying to do. count must always be > 0 at this point and count seems to refer to some limit on the maximum number of items to return, so I'm not sure why the length of the string is being compared to count here, unless count is in terms of characters here. If that's the case, it also seems like the sb.append call above needs to account for count, but that code's commented out. However, I think this whole thing can be simplified into just a loop that starts at index 0, rather than getting a single element and then having a loop to look for other elements.
In fact, it seems like all the other cases above can and should be simplified the same way, but that's completely separate to this PR.
There was a problem hiding this comment.
Can you convert this comment to an issue?
There was a problem hiding this comment.
Probably a cut-n-paste issue
There was a problem hiding this comment.
Looks like that is copied from the else clause, which also had that commented out.
There was a problem hiding this comment.
setDataValue is definitely passing in a string.
The others might need to be fixed.
src/org.hdfgroup.hdfview/hdf/view/TableView/DataProviderFactory.java
Outdated
Show resolved
Hide resolved
| * @param bufObject the data object | ||
| * @param newValue the new data object | ||
| */ | ||
| public void setDataValue(int columnIndex, int rowIndex, Object bufObject, Object newValue) |
There was a problem hiding this comment.
I'm not sure that we actually need to implement this setDataValue variant here, and it may actually be problematic to do so, based on my old comment in the Javadoc above: "this method should, in general, not be called by atomic type DataProviders.".
There was a problem hiding this comment.
Maybe, but container types will need the same method if I'm correct. Will need a test file.
| if (isFLT16) | ||
| newbufObject = Float.float16ToFloat((short)bufObject); | ||
| try { | ||
| setDataValue(index, newbufObject, newValue); |
There was a problem hiding this comment.
I think this setDataValue call is just recursively calling itself, which will lead to a stack overflow when editing float16 values inside an array type. I do believe we need this setDataValue variant though to support array types.
There was a problem hiding this comment.
Yep, exactly what happened.
Still need tests