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

Jump to bottom

H5Ztrans: (feature) Improved H5Z_xform_noop() and H5Z_xform_create() … #933

Merged
merged 5 commits into from
Sep 13, 2021
Merged

H5Ztrans: (feature) Improved H5Z_xform_noop() and H5Z_xform_create() … #933

merged 5 commits into from
Sep 13, 2021

Conversation

jwsblokland
Copy link
Contributor

Improved H5Z_xform_noop() and H5Z_xform_create() function

  • Made a small improvement for H5Z_xform_noop() function. Now,
    the function will also return TRUE if the data transform function
    expression = "x". For this case, the HDF5 library behaves in a
    similar fashion as the case when no data transform function has been
    specified.
  • Improved the inline documentation of the function
    H5Z_xform_create() such it is more inline with the rest of the
    code.

This pull request has been requested in the abandoned pull request #922.

qkoziol
qkoziol requested changes Aug 19, 2021
View reviewed changes
Copy link
Contributor

@qkoziol qkoziol left a comment

Choose a reason for hiding this comment

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

This code looks fine, but it needs a test, to demonstrate that it actually works. Adding it to both the serial testing for data transforms (in the 'test' directory) and the parallel testing (in the 'testpar' directory) is probably necessary, since I believe that use in parallel HDF5 is desired.

@qkoziol
Copy link
Contributor

qkoziol commented Aug 19, 2021

This code looks fine, but it needs a test, to demonstrate that it actually works. Adding it to both the serial testing for data transforms (in the 'test' directory) and the parallel testing (in the 'testpar' directory) is probably necessary, since I believe that use in parallel HDF5 is desired.

Also, with a test, you'll guarantee that it stays working. :-)

@jwsblokland jwsblokland requested a review from rawarren as a code owner August 20, 2021 13:02
@jwsblokland
Copy link
Contributor Author

jwsblokland commented Aug 20, 2021 •
edited
Loading

As requested I have added 1 serial test and 2 parallel tests. I must admit usually I add some tests to demostrates the feature or bug fix works as expected. Apparently, this case I did not, so thank you for reminding me to add these tests.

Jan-Willem Blokland and others added 4 commits August 20, 2021 15:45
H5Ztrans: (feature) Improved H5Z_xform_noop() and H5Z_xform_create() …
c0617f6 
…function

- Made a small improvement for H5Z_xform_noop() function. Now,
  the function will also return TRUE if the data transform function
  expression = "x". For this case, the HDF5 library behaves in a
  similar fashion as the case when no data transform function has been
  specified.
- Improved the inline documentation of the function
  H5Z_xform_create() such it is more inline with the rest of the
  code.
@github-actions
Committing clang-format changes
8614a37
H5Ztrans: (feature) Added 3 tests for improved H5Z_xform_noop() function
5caf192 
- Added serial test with data transform expression = "x" to
  verify the improved H5Z_xform_noop() function behaves as expected.
- Added 2 parallel tests with data transform expression = "x"
  in combination with a filter. Before, these tests will fail but
  with the improved H5Z_xform_noop() function they work and result
  in the expected behavior.
- Small bug fix for one of parallel filter tests.
@github-actions
Committing clang-format changes
b05dec6
@jwsblokland jwsblokland force-pushed the feature/data_transform branch from ce53f3d to b05dec6 Compare August 20, 2021 13:45
@jwsblokland jwsblokland requested a review from qkoziol August 20, 2021 14:16
epourmal
epourmal reviewed Sep 8, 2021
View reviewed changes
testpar/t_filters_parallel.c
* the transform function. Therefore it does not apply
* the transform function resulting in not breaking to
* independent I/O.
*
Copy link

Choose a reason for hiding this comment

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

Please add the comment to the RELEASE.txt file (similar to what you have here) explaining the change. It should go under Bug Fixed since HDF5 ... / Library.
Thank you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@epourmal , I have added a comment about the implemented improvement to the RELEASE.txt file. Thanks for mentioning this.

Copy link

Choose a reason for hiding this comment

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

Thank you! We are currently testing on HPC systems now and will accept the change.

H5Ztrans: (feature) Added release note about detection of the
feb8991 
           simple data transform function "x".

- Added a brief explanation about the implemented improvement
  of the detection of the simple data transform function "x"
  to the RELEASE.txt file.
@lrknox
Copy link
Collaborator

lrknox commented Sep 13, 2021

The tests added also passed on jelly, lassen quartz and mutrino.

@lrknox lrknox merged commit 7c973de into HDFGroup:develop Sep 13, 2021
This was referenced Sep 14, 2021
Merged
Merged
lrknox added a commit that referenced this pull request Sep 14, 2021
@lrknox @github-actions
H5Ztrans: (feature) Improved H5Z_xform_noop() and H5Z_xform_create() … (
3cf0d48 
#933) (#1007)

* H5Ztrans: (feature) Improved H5Z_xform_noop() and H5Z_xform_create() function

- Made a small improvement for H5Z_xform_noop() function. Now,
  the function will also return TRUE if the data transform function
  expression = "x". For this case, the HDF5 library behaves in a
  similar fashion as the case when no data transform function has been
  specified.
- Improved the inline documentation of the function
  H5Z_xform_create() such it is more inline with the rest of the
  code.

* Committing clang-format changes

* H5Ztrans: (feature) Added 3 tests for improved H5Z_xform_noop() function

- Added serial test with data transform expression = "x" to
  verify the improved H5Z_xform_noop() function behaves as expected.
- Added 2 parallel tests with data transform expression = "x"
  in combination with a filter. Before, these tests will fail but
  with the improved H5Z_xform_noop() function they work and result
  in the expected behavior.
- Small bug fix for one of parallel filter tests.

* Committing clang-format changes

* H5Ztrans: (feature) Added release note about detection of the
           simple data transform function "x".

- Added a brief explanation about the implemented improvement
  of the detection of the simple data transform function "x"
  to the RELEASE.txt file.

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>

Co-authored-by: Jan-Willem Blokland <Jan-Willem.Blokland@Shell.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
lrknox added a commit that referenced this pull request Sep 14, 2021
@lrknox @github-actions
H5Ztrans: (feature) Improved H5Z_xform_noop() and H5Z_xform_create() … (
d110a16 
#933) (#1008)

* H5Ztrans: (feature) Improved H5Z_xform_noop() and H5Z_xform_create() function

- Made a small improvement for H5Z_xform_noop() function. Now,
  the function will also return TRUE if the data transform function
  expression = "x". For this case, the HDF5 library behaves in a
  similar fashion as the case when no data transform function has been
  specified.
- Improved the inline documentation of the function
  H5Z_xform_create() such it is more inline with the rest of the
  code.

* Committing clang-format changes

* H5Ztrans: (feature) Added 3 tests for improved H5Z_xform_noop() function

- Added serial test with data transform expression = "x" to
  verify the improved H5Z_xform_noop() function behaves as expected.
- Added 2 parallel tests with data transform expression = "x"
  in combination with a filter. Before, these tests will fail but
  with the improved H5Z_xform_noop() function they work and result
  in the expected behavior.
- Small bug fix for one of parallel filter tests.

* Committing clang-format changes

* H5Ztrans: (feature) Added release note about detection of the
           simple data transform function "x".

- Added a brief explanation about the implemented improvement
  of the detection of the simple data transform function "x"
  to the RELEASE.txt file.

Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>

Co-authored-by: Jan-Willem Blokland <Jan-Willem.Blokland@Shell.com>
Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
@jwsblokland jwsblokland deleted the feature/data_transform branch September 13, 2023 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@epourmal epourmal epourmal left review comments

@lrknox lrknox lrknox approved these changes

@jhendersonHDF jhendersonHDF jhendersonHDF approved these changes

@derobins derobins Awaiting requested review from derobins derobins is a code owner

@fortnern fortnern Awaiting requested review from fortnern fortnern is a code owner

@jrmainzer jrmainzer Awaiting requested review from jrmainzer

@soumagne soumagne Awaiting requested review from soumagne

@vchoi-hdfgroup vchoi-hdfgroup Awaiting requested review from vchoi-hdfgroup vchoi-hdfgroup is a code owner

@rawarren rawarren Awaiting requested review from rawarren

@qkoziol qkoziol Awaiting requested review from qkoziol qkoziol is a code owner

@bmribler bmribler Awaiting requested review from bmribler bmribler is a code owner

@byrnHDF byrnHDF Awaiting requested review from byrnHDF byrnHDF is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jwsblokland @qkoziol @lrknox @epourmal @jhendersonHDF