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.

Sign up for GitHub

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 support for float16 datatypes #179

Merged
merged 17 commits into from
May 3, 2024
Merged

Add support for float16 datatypes #179

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
e3d2604
Add support for float16 datatypes
byrnHDF Apr 18, 2024
55f8b2b
Committing clang-format changes
github-actions[bot] Apr 18, 2024
d400999
StringAttribute dialog is used for non-HDF5 attributes
byrnHDF Apr 25, 2024
af49dd3
Add float16 test and fix dataDisplay
byrnHDF Apr 26, 2024
d94f8db
Committing clang-format changes
github-actions[bot] Apr 26, 2024
7ab8c4d
Fix comments and use same float16 conversion method
byrnHDF Apr 26, 2024
3fd3303
Correct merge issues
byrnHDF Apr 26, 2024
4d65adc
Committing clang-format changes
github-actions[bot] Apr 26, 2024
06b420b
Comment edit
byrnHDF Apr 26, 2024
eba8d7c
Committing clang-format changes
github-actions[bot] Apr 26, 2024
0ca1e57
Call setValue() up the stack
byrnHDF Apr 27, 2024
68ddb7c
Committing clang-format changes
github-actions[bot] Apr 27, 2024
94b691f
seDataValue requires the newValue Object to be a string
byrnHDF May 1, 2024
0f0d9bb
Committing clang-format changes
github-actions[bot] May 1, 2024
db4a741
Add tests for compound and array of float16 values
byrnHDF May 1, 2024
640ca0e
Committing clang-format changes
github-actions[bot] May 1, 2024
af44070
Merge branch 'master' into master-float16
byrnHDF May 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions build.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -831,11 +831,11 @@
</sequential>
</macrodef>

<target name="runauitest" description="Runs the test you specify on the command line with -Dtest=" depends="jar, clean-junit-uitest, compile-test, ensure-test5-name, ensure-test4-name, jacoco">
<target name="runauitest" description="Runs the test you specify on the command line with -Dtest=" depends="deploy,jar, clean-junit-uitest, compile-test, ensure-test5-name, ensure-test4-name, jacoco">
<mkdir dir="${report.dir}" />

<run-ui-junit
rootdir="${basedir}/${uitestclasses.dir}"
rootdir="${basedir}/${jar.dir}"
workdir="${basedir}/${uitestclasses.dir}/uitest"
propfile="${basedir}/${uitestclasses.dir}/.hdfview${app.version}"
includes="**/${uitestclasses.dir}/${test}.class"
Expand All @@ -851,7 +851,7 @@
<mkdir dir="${report.dir}" />

<run-ui-junit
rootdir="${basedir}/${uitestclasses.dir}"
rootdir="${basedir}/${jar.dir}"
workdir="${basedir}/${uitestclasses.dir}/uitest"
propfile="${basedir}/${uitestclasses.dir}/.hdfview${app.version}"
includes="**/${uitestclasses.dir}/${test}.class"
Expand All @@ -865,7 +865,7 @@

<!-- Run the top-level HDF4 UI tests -->
<run-ui-junit
rootdir="${basedir}/${hdf4uitestclasses.dir}"
rootdir="${basedir}/${jar.dir}"
workdir="${basedir}/${hdf4uitestclasses.dir}"
propfile="${basedir}/${uitestclasses.dir}/.hdfview${app.version}"
includes="**/${hdf4uitestclasses.dir}/${test}.class"
Expand All @@ -879,7 +879,7 @@

<!-- Run the top-level HDF5 UI tests -->
<run-ui-junit
rootdir="${basedir}/${hdf5uitestclasses.dir}"
rootdir="${basedir}/${jar.dir}"
workdir="${basedir}/${hdf5uitestclasses.dir}"
propfile="${basedir}/${uitestclasses.dir}/.hdfview${app.version}"
includes="${hdf5uitestclasses.dir}/${test}.class"
Expand All @@ -891,7 +891,7 @@
<mkdir dir="${report.dir}" />

<run-ui-junit
rootdir="${basedir}/${testclasses.dir}"
rootdir="${basedir}/${jar.dir}"
workdir="${basedir}/${testclasses.dir}/modules"
includes="**/modules/*Test*"
excludes="**/modules/*$*"
Expand Down
14 changes: 12 additions & 2 deletions src/org.hdfgroup.hdfview/hdf/view/TableView/DataDisplayConverterFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -712,6 +712,7 @@ private static class NumericalDataDisplayConverter extends HDFDisplayConverter {
private final StringBuilder buffer;
private final long typeSize;
private final boolean isUINT64;
private final boolean isFLT16;

NumericalDataDisplayConverter(final Datatype dtype) throws Exception
{
Expand All @@ -727,6 +728,7 @@ private static class NumericalDataDisplayConverter extends HDFDisplayConverter {

typeSize = dtype.getDatatypeSize();
isUINT64 = dtype.isUnsigned() && (typeSize == 8);
isFLT16 = dtype.isFloat() && (typeSize == 2);
}

@Override
Expand Down Expand Up @@ -758,10 +760,18 @@ else if (showAsBin) {
buffer.append(Tools.toBinaryString(Long.valueOf(value.toString()), (int)typeSize));
}
else if (numberFormat != null) {
buffer.append(numberFormat.format(value));
log.trace("canonicalToDisplayValue({}): numberFormat", value);
if (isFLT16)
buffer.append(numberFormat.format((float)value));
byrnHDF marked this conversation as resolved.
Show resolved Hide resolved
else
buffer.append(numberFormat.format(value));
}
else {
buffer.append(value.toString());
log.trace("canonicalToDisplayValue({}): else", value);
if (isFLT16)
buffer.append(Float.toString((float)value));
else
buffer.append(value.toString());
}
}
catch (Exception ex) {
Expand Down
87 changes: 84 additions & 3 deletions src/org.hdfgroup.hdfview/hdf/view/TableView/DataProviderFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ private static final HDFDataProvider getDataProvider(final Datatype dtype, final
final boolean dataTransposed) throws Exception
{
HDFDataProvider dataProvider = null;
log.debug("getDataProvider(): Datatype is {}", dtype.getDescription());

try {
if (dtype.isCompound())
Expand Down Expand Up @@ -1623,7 +1624,7 @@ private static class NumericalDataProvider extends HDFDataProvider {
private static final Logger log = LoggerFactory.getLogger(NumericalDataProvider.class);

private final boolean isUINT64;

private final boolean isFLT16;
private final long typeSize;

NumericalDataProvider(final Datatype dtype, final Object dataBuf, final boolean dataTransposed)
Expand All @@ -1633,6 +1634,7 @@ private static class NumericalDataProvider extends HDFDataProvider {

typeSize = dtype.getDatatypeSize();
isUINT64 = dtype.isUnsigned() && (typeSize == 8);
isFLT16 = dtype.isFloat() && (typeSize == 2);
}

@Override
Expand All @@ -1641,7 +1643,10 @@ public Object getDataValue(int columnIndex, int rowIndex)
super.getDataValue(columnIndex, rowIndex);

try {
if (isUINT64)
if (isFLT16)
// theValue = Float.toString(Float.float16ToFloat((short)theValue));
byrnHDF marked this conversation as resolved.
Show resolved Hide resolved
theValue = Float.float16ToFloat((short)theValue);
else if (isUINT64)
theValue = Tools.convertUINT64toBigInt(Long.valueOf((long)theValue));
}
catch (Exception ex) {
Expand All @@ -1660,7 +1665,10 @@ public Object getDataValue(Object obj, int index)
super.getDataValue(obj, index);

try {
if (isUINT64)
if (isFLT16)
// theValue = Float.toString(Float.float16ToFloat((short)theValue));
theValue = Float.float16ToFloat((short)theValue);
else if (isUINT64)
theValue = Tools.convertUINT64toBigInt(Long.valueOf((long)theValue));
}
catch (Exception ex) {
Expand All @@ -1672,6 +1680,79 @@ public Object getDataValue(Object obj, int index)

return theValue;
}

/**
* update the data value of a compound type.
*
* @param columnIndex the column
* @param rowIndex the row
* @param newValue the new data value object
*/
@Override
public void setDataValue(int columnIndex, int rowIndex, Object newValue)
{
Object theValue = newValue;
if (isFLT16)
theValue = Short.toString(Float.floatToFloat16(Float.parseFloat((String)newValue)));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this set as a string here rather than a real data value?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be an artifact before the Display/Converter changes?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did test a change value and it worked - will need to check into container types.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe - I know not having it failed the first time I tried - will try again since changing other code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to be a string - fails without it

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@byrnHDF byrnHDF Apr 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The trouble is that float16 is a Short underneath and everything was triggering on that type.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


super.setDataValue(columnIndex, rowIndex, theValue);
}

/**
* When a CompoundDataProvider wants to pass a List of data down to a nested CompoundDataProvider, or
* when a top-level container DataProvider (such as an ArrayDataProvider) wants to hand data down to a
* base CompoundDataProvider, we need to pass down a List of data and the new value, plus a field and
* row index. This method is for facilitating this behavior.
*
* In general, all "container" DataProviders that have a "container" base DataProvider should call
* down into their base DataProvider(s) using this{}, method, in order to ensure that buried
* CompoundDataProviders get handled correctly. When their base DataProvider is not a "container"
* type, the method setDataValue(index, Object, Object) should be used instead.
*
* For atomic type DataProviders, we treat this method as directly calling into setDataValue(index,
* Object, Object) using the passed rowIndex. However, this method should, in general, not be called
* by atomic type DataProviders.
*
* @param columnIndex the column
* @param rowIndex the row
* @param bufObject the data object
* @param newValue the new data object
*/
public void setDataValue(int columnIndex, int rowIndex, Object bufObject, Object newValue)
Copy link
Collaborator

@jhendersonHDF jhendersonHDF Apr 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.".

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe, but container types will need the same method if I'm correct. Will need a test file.

{
Object newbufObject = bufObject;
if (isFLT16)
newbufObject = Float.float16ToFloat((short)bufObject);
setDataValue(rowIndex, newbufObject, newValue);
}

/**
* When a parent HDFDataProvider (such as an ArrayDataProvider) wants to set a data value by routing
* the operation through its base HDFDataProvider, the parent HDFDataProvider will generally know the
* direct index to have the base provider use. This method is to facilitate this kind of behavior.
*
* Note that this method takes two Object parameters, one which is the object that the method should
* set its data inside of and one which is the new value to set. This is to be able to nicely support
* nested compound DataProviders.
*
* @param index the index into the data array
* @param bufObject the data object
* @param newValue the new data object
*/
public void setDataValue(int index, Object bufObject, Object newValue)
{
Object newbufObject = bufObject;
if (isFLT16)
newbufObject = Float.float16ToFloat((short)bufObject);
try {
setDataValue(index, newbufObject, newValue);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, exactly what happened.

}
catch (Exception ex) {
log.debug("setDataValue({}, {})=({}): updateAtomicValue failure: ", index, newbufObject,
newValue, ex);
}
log.trace("setDataValue({}, {})=({}): finish", index, newbufObject, newValue);
}
}

private static class EnumDataProvider extends HDFDataProvider {
Expand Down
28 changes: 18 additions & 10 deletions src/org.hdfgroup.hdfview/hdf/view/TableView/DataValidatorFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -636,19 +636,27 @@ public boolean validate(int colIndex, int rowIndex, Object newValue)
break;

case 2:
if (datasetDatatype.isUnsigned()) {
/*
* First try to parse as a larger type in order to catch a NumberFormatException
*/
Integer intValue = Integer.parseInt((String)newValue);
if (intValue < 0)
throw new NumberFormatException("Invalid negative value for unsigned datatype");
if (datasetDatatype.isInteger()) {
if (datasetDatatype.isUnsigned()) {
/*
* First try to parse as a larger type in order to catch a NumberFormatException
*/
Integer intValue = Integer.parseInt((String)newValue);
if (intValue < 0)
throw new NumberFormatException(
"Invalid negative value for unsigned datatype");

if (intValue > (Short.MAX_VALUE * 2) + 1)
throw new NumberFormatException("Value out of range. Value:\"" + newValue + "\"");
if (intValue > (Short.MAX_VALUE * 2) + 1)
throw new NumberFormatException("Value out of range. Value:\"" + newValue +
"\"");
}
else {
Short.parseShort((String)newValue);
}
}
else {
Short.parseShort((String)newValue);
/* Floating-point type */
Float.parseFloat((String)newValue);
}
break;

Expand Down
16 changes: 13 additions & 3 deletions src/org.hdfgroup.hdfview/hdf/view/dialog/NewCompoundAttributeDialog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,11 +78,13 @@ public class NewCompoundAttributeDialog extends NewDataObjectDialog {
"unsigned short (16-bit)", // 4
"unsigned int (32-bit)", // 5
"long (64-bit)", // 6
"float", // 7
"double", // 8
"float (32-bit)", // 7
"double (64-bit)", // 8
"string", // 9
"enum", // 10
"unsigned long (64-bit)" // 11
"unsigned long (64-bit)", // 11
"float16 (16-bit)", // 12
"long double (128-bit)" // 13
jhendersonHDF marked this conversation as resolved.
Show resolved Hide resolved
jhendersonHDF marked this conversation as resolved.
Show resolved Hide resolved
};

private Combo nFieldBox, templateChoice;
Expand Down Expand Up @@ -465,6 +467,14 @@ else if (DATATYPE_NAMES[11].equals(typeName)) {
type = fileFormat.createDatatype(Datatype.CLASS_INTEGER, 8, Datatype.NATIVE,
Datatype.SIGN_NONE);
}
else if (DATATYPE_NAMES[12].equals(typeName)) {
type =
fileFormat.createDatatype(Datatype.CLASS_FLOAT, 2, Datatype.NATIVE, Datatype.NATIVE);
}
else if (DATATYPE_NAMES[13].equals(typeName)) {
type =
fileFormat.createDatatype(Datatype.CLASS_FLOAT, 16, Datatype.NATIVE, Datatype.NATIVE);
}
else {
throw new IllegalArgumentException("Invalid data type.");
}
Expand Down
Loading