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

Incorporate support for new CDF-5 external data types in SCORPIO #525

Merged
merged 4 commits into from
Jan 31, 2024

Conversation

bartgol
Copy link
Contributor

@bartgol bartgol commented Jun 30, 2023

As an extension of CDF-2, CDF-5 format has additional external
data types, namely NC_UBYTE, NC_USHORT, NC_UINT, NC_INT64,
and NC_UINT64.

This PR includes support for these new CDF-5 data types in
SCORPIO, across all calls and IO types. Additionally, the C
unit tests have been updated to include tests for these types.

@bartgol bartgol added the bugfix label Jun 30, 2023
@bartgol bartgol requested a review from dqwu June 30, 2023 16:43
@jayeshkrishna jayeshkrishna added enhancement Next Release Enhancements slated for the upcoming (next) release and removed bugfix labels Jul 11, 2023
@bartgol
Copy link
Contributor Author

bartgol commented Nov 20, 2023

@jayeshkrishna @dqwu I'm going through PRs/issues I have open, and found this one. Any thought of integrating it?

@jayeshkrishna
Copy link
Contributor

jayeshkrishna commented Nov 20, 2023

@dqwu : There are some things to add to this PR before merging it,

  • Ensure that NC_INT64 support is available for all calls/IO types
  • Add support for NC_UINT64
  • Ensure that the tests are updated to test these types
  • Add a configure check that long long ints are 64-bit

@dqwu: You can create a new branch off master, cherry pick the commit in this PR and add further commits to fix the issues above

@dqwu dqwu force-pushed the bartgol/get_var-add-missing-int64-case branch from 7c9e048 to b131af9 Compare December 6, 2023 22:09
@dqwu dqwu changed the base branch from develop to master December 6, 2023 23:24
@dqwu dqwu force-pushed the bartgol/get_var-add-missing-int64-case branch from b131af9 to f271470 Compare December 7, 2023 17:28
@dqwu dqwu changed the title Add int64 support for pnetcdf backend in PIOc_get_var Incorporate support for new CDF-5 external data types in SCORPIO Dec 14, 2023
@dqwu dqwu requested a review from jayeshkrishna December 14, 2023 23:55
@dqwu
Copy link
Contributor

dqwu commented Dec 14, 2023

@jayeshkrishna Please re-review this updated PR, thanks.

@dqwu dqwu added the ADIOS All ADIOS related issues/enhancements label Dec 14, 2023
@jayeshkrishna
Copy link
Contributor

jayeshkrishna commented Dec 15, 2023

We would also need the configure checks for ensuring "long long"s are 64-bit for example. If not, we need to use a 64-bit datatype to write the data out

@jayeshkrishna
Copy link
Contributor

Also, we will need to add support for the corresponding Fortran types (NF_*)

@dqwu
Copy link
Contributor

dqwu commented Dec 15, 2023

We would also need the configure checks for ensuring "long long"s are 64-bit for example. If not, we need to use a 64-bit datatype to write the data out

We already have this configure check in this PR. Also, PnetCDF uses "longlong" for API names and C type "long long" for API arguments to support 64-bit data types.

@dqwu
Copy link
Contributor

dqwu commented Dec 15, 2023

Also, we will need to add support for the corresponding Fortran types (NF_*)

I think we can support that in another PR. Also note that existing Fortran unit tests only use PIO_TF_DATA_TYPE = "PIO_int" and PIO_TF_FC_DATA_TYPE = "integer" (no 64-bit int types).

@jayeshkrishna
Copy link
Contributor

We would also need the configure checks for ensuring "long long"s are 64-bit for example. If not, we need to use a 64-bit datatype to write the data out

We already have this configure check in this PR. Also, PnetCDF uses "longlong" for API names and C type "long long" for API arguments to support 64-bit data types.

Ok, at least the way I see it the NetCDF type has a fixed size while a C long long does not (although it has a min size). I just don't want the configure to fail (on a new system/compiler) and end up in a situation where we have not way to disable it

@jayeshkrishna
Copy link
Contributor

jayeshkrishna commented Dec 15, 2023

Also, we will need to add support for the corresponding Fortran types (NF_*)

I think we can support that in another PR. Also note that existing Fortran unit tests only use PIO_TF_DATA_TYPE = "PIO_int" and PIO_TF_FC_DATA_TYPE = "integer" (no 64-bit int types).

Ok, without this fix only the C interface has access to the new types (and we might drop/forget adding support for the Fortran types when we get busy with other issues). The unit testing framework can be expanded to cover more types, right?

Copy link
Contributor

@jayeshkrishna jayeshkrishna left a comment

Choose a reason for hiding this comment

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

Please go ahead and merge this PR. The Fortran interface updates can be made in a separate PR (create an issue for it so that it does not fall off our radar)

bartgol and others added 4 commits January 30, 2024 15:54
Starting from version 4.4.0, the NetCDF library supports the CDF-5
file format for both sequential and parallel file access.

Starting from version 1.3.0, the PnetCDF library also supports the
CDF-5 file format.

As an extension of CDF-2, CDF-5 format supports the following new
external data types:
NC_UBYTE, NC_USHORT, NC_UINT, NC_INT64, and NC_UINT64

Update the unit tests to ensure that these new external data types
are available for all calls and IO types.
Fix handling of new CDF-5 external data types in PnetCDF and ADIOS
IO types.

Remove incorrect assumption that these data types are only supported
by NetCDF4.
Introduce a configure check to verify that long long ints are
genuinely 64-bit, a requirement for supporting the 64-bit data
format (CDF-5) in NetCDF and PnetCDF libraries.
@dqwu dqwu force-pushed the bartgol/get_var-add-missing-int64-case branch from 4dccb53 to 283eb6f Compare January 30, 2024 21:55
dqwu added a commit that referenced this pull request Jan 30, 2024
…#525)

As an extension of CDF-2, CDF-5 format has additional external
data types, namely NC_UBYTE, NC_USHORT, NC_UINT, NC_INT64,
and NC_UINT64.

This PR includes support for these new CDF-5 data types in
SCORPIO, across all calls and IO types. Additionally, the C
unit tests have been updated to include tests for these types.

* bartgol/get_var-add-missing-int64-case:
  Configure check for 64-bit long long ints
  Enhancements for new CDF-5 external data types
  Test the new external data types introduced by the CDF-5 file format
  Add missing handling of int64 case for pnetcdf in PIOc_get_var
@dqwu dqwu merged commit 1238a73 into master Jan 31, 2024
@dqwu dqwu deleted the bartgol/get_var-add-missing-int64-case branch January 31, 2024 18:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ADIOS All ADIOS related issues/enhancements enhancement Next Release Enhancements slated for the upcoming (next) release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants