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

Tools long double updates #522

Merged
merged 160 commits into from
Apr 15, 2021
Merged

Tools long double updates #522

merged 160 commits into from
Apr 15, 2021

Conversation

byrnHDF
Copy link
Contributor

@byrnHDF byrnHDF commented Mar 26, 2021

Added tests for tools and java testing.

byrnHDF added 30 commits January 7, 2021 07:15
@lrknox
Copy link
Collaborator

lrknox commented Apr 6, 2021

This is the output on jelly and hedgehog from h5dump of tldouble.h5 with current source from Allen's fork:

Jelly (little endian) output
[lrknox@jelly tools]$ src/h5dump/.libs/h5dump ../../tools/testfiles/tldouble.h5
HDF5 "../../tools/testfiles/tldouble.h5" {
GROUP "/" {
DATASET "dset" {
DATATYPE H5T_NATIVE_LDOUBLE
DATASPACE SIMPLE { ( 3 ) / ( 3 ) }
DATA {
(0): 1, 2, 3
}
...

hedgehog (big endian) output
-bash-5.0$ src/h5dump/.libs/h5dump ../../tools/testfiles/tldouble.h5
HDF5 "../../tools/testfiles/tldouble.h5" {
GROUP "/" {
DATASET "dset" {
DATATYPE 128-bit little-endian floating-point
DATASPACE SIMPLE { ( 3 ) / ( 3 ) }
DATA {
(0): 1, 2, 3
}
...

@epourmal
Copy link

epourmal commented Apr 6, 2021 via email

@byrnHDF
Copy link
Contributor Author

byrnHDF commented Apr 6, 2021

Yes the output is correct and the test should pass.
This was the conclusion of the developers on Friday, that the tools should print NATIVE if the datatype matches that platform's NATIVE type, else print what the library knows about the type.
I might need to add more characteristics like precision.

@epourmal
Copy link

epourmal commented Apr 6, 2021 via email

@byrnHDF
Copy link
Contributor Author

byrnHDF commented Apr 7, 2021

I will revert back to the removal of NATIVE_LDOUBLE in h5dump and h5ls. I will create an issue for the other native types in tools.
I will also add a precision value to the output of integers and floats.

@byrnHDF
Copy link
Contributor Author

byrnHDF commented Apr 8, 2021

The long double h5dump output now appears as:
DATATYPE 128-bit little-endian floating-point 80-bit precision

@lrknox
Copy link
Collaborator

lrknox commented Apr 13, 2021

The long double h5dump output now appears as:
DATATYPE 128-bit little-endian floating-point 80-bit precision

@koziol does this change satisfy your request?

@byrnHDF
Copy link
Contributor Author

byrnHDF commented Apr 13, 2021

The long double h5dump output now appears as:
DATATYPE 128-bit little-endian floating-point 80-bit precision

Also note that the n-bit h5dump test now shows that the datatype is of 3-bit precision:
DATATYPE 32-bit little-endian integer 3-bit precision

/* print size, order, and sign */
h5tools_str_append(buffer, "%lu-bit%s%s integer", (unsigned long)(8 * H5Tget_size(type)),
order_s, sign_s);
/* print size, order, sign, and precision */
Copy link
Contributor

Choose a reason for hiding this comment

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

This is helpful, and probably sufficient for integers.

/* print size and byte order */
h5tools_str_append(buffer, "%lu-bit%s floating-point", (unsigned long)(8 * H5Tget_size(type)),
order_s);
/* print size. byte order, and precision */
Copy link
Contributor

Choose a reason for hiding this comment

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

This is helpful, and not sufficient for floating-point types. There's a lot more details for floating point types: mantissa, sign bit, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but how much detail is needed? h5ls does print more and I could make h5dump match h5ls - at the very least.

Copy link
Contributor

Choose a reason for hiding this comment

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

What's the output from h5ls look like?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Opened "tldouble.h5" with sec2 driver.
dset Dataset {3/3}
Location: 1:800
Links: 1
Storage: 48 logical bytes, 48 allocated bytes, 100.00% utilization
Type: 128-bit little-endian floating-point
(80 bits of precision beginning at bit 0)
(48 zero bits at bit 80)
(significant for 64 bits at bit 0, no normalization)
(exponent for 15 bits at bit 64, bias is 0x3fff)
(sign bit at 79)

@qkoziol
Copy link
Contributor

qkoziol commented Apr 13, 2021

The long double h5dump output now appears as:
DATATYPE 128-bit little-endian floating-point 80-bit precision

@koziol does this change satisfy your request?

Some of this is in the right direction (more information for custom atomic types), but the H5T_NATIVE_LDOUBLE issue still isn't resolved. And now there's doxygen and Perl changes that should be in another PR, not here.

@byrnHDF
Copy link
Contributor Author

byrnHDF commented Apr 13, 2021

The long double h5dump output now appears as:
DATATYPE 128-bit little-endian floating-point 80-bit precision

@koziol does this change satisfy your request?

Some of this is in the right direction (more information for custom atomic types), but the H5T_NATIVE_LDOUBLE issue still isn't resolved.

According to Elena, NATIVE is a memory type. She has stated that h5dump/h5ls should display the file type. That reasoning suggests that h5dump/h5ls should eliminate the display of the NATIVE types and always show the file type. I agree since the NATIVE type is dependent on platform/compiler.

And now there's doxygen and Perl changes that should be in another PR, not here.

Well this PR is taking so long, I accidentally committed files!

@qkoziol
Copy link
Contributor

qkoziol commented Apr 13, 2021

The long double h5dump output now appears as:
DATATYPE 128-bit little-endian floating-point 80-bit precision

@koziol does this change satisfy your request?

Some of this is in the right direction (more information for custom atomic types), but the H5T_NATIVE_LDOUBLE issue still isn't resolved.

According to Elena, NATIVE is a memory type. She has stated that h5dump/h5ls should display the file type. That reasoning suggests that h5dump/h5ls should eliminate the display of the NATIVE types and always show the file type. I agree since the NATIVE type is dependent on platform/compiler.

That suggests that all the "native" datatype output should be removed. Is that true?

@qkoziol
Copy link
Contributor

qkoziol commented Apr 13, 2021

And now there's doxygen and Perl changes that should be in another PR, not here.

Well this PR is taking so long, I accidentally committed files!

OK, please back out the accidental commit then.

@epourmal
Copy link

The long double h5dump output now appears as:
DATATYPE 128-bit little-endian floating-point 80-bit precision

@koziol does this change satisfy your request?

Some of this is in the right direction (more information for custom atomic types), but the H5T_NATIVE_LDOUBLE issue still isn't resolved.

According to Elena, NATIVE is a memory type. She has stated that h5dump/h5ls should display the file type. That reasoning suggests that h5dump/h5ls should eliminate the display of the NATIVE types and always show the file type. I agree since the NATIVE type is dependent on platform/compiler.

That suggests that all the "native" datatype output should be removed. Is that true?

Please remove NATIVE type from tools' output and merge the fix.

HDF5 tools display properties of data as stored in the file. If someone thinks that "NATIVE" can be used to describe portable data in the file, please justify, how "NATIVE" is portable. Thank you!

@qkoziol
Copy link
Contributor

qkoziol commented Apr 13, 2021

The long double h5dump output now appears as:
DATATYPE 128-bit little-endian floating-point 80-bit precision

@koziol does this change satisfy your request?

Some of this is in the right direction (more information for custom atomic types), but the H5T_NATIVE_LDOUBLE issue still isn't resolved.

According to Elena, NATIVE is a memory type. She has stated that h5dump/h5ls should display the file type. That reasoning suggests that h5dump/h5ls should eliminate the display of the NATIVE types and always show the file type. I agree since the NATIVE type is dependent on platform/compiler.

That suggests that all the "native" datatype output should be removed. Is that true?

Please remove NATIVE type from tools' output and merge the fix.

HDF5 tools display properties of data as stored in the file. If someone thinks that "NATIVE" can be used to describe portable data in the file, please justify, how "NATIVE" is portable. Thank you!

I think that's good reasoning. :-) (Although a lot more work on this PR before it's merged)

@byrnHDF
Copy link
Contributor Author

byrnHDF commented Apr 13, 2021

I agree, remove NATIVE display output.
Removed non long double concerned files.
Q: Should this be merged and then followup with a PR to remove the other NATIVE display output or combine both?

@byrnHDF
Copy link
Contributor Author

byrnHDF commented Apr 14, 2021

Created issue #551 for fixing tools to only display file datatypes.
Also created HDFFV-11236 for the h5dump BNF specification.

@lrknox lrknox merged commit e21f7aa into HDFGroup:develop Apr 15, 2021
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.

5 participants