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

Update mppnccombine.c to V2.2.8 #326

Merged

Conversation

HansVahlenkamp
Copy link
Contributor

If no netCDF format is specified (no -64 or -n4 options) for the output file then use the netCDF format of the first input file; use netCDF3 64-bit offset format or netCDF4 classic format.

@underwoo
Copy link
Member

@HansVahlenkamp is this ready? If so, please remove the "draft" status of this PR.

@HansVahlenkamp HansVahlenkamp marked this pull request as ready for review December 10, 2024 15:07
@HansVahlenkamp
Copy link
Contributor Author

@HansVahlenkamp is this ready? If so, please remove the "draft" status of this PR.

It passes my testing.

Copy link
Member

@underwoo underwoo left a comment

Choose a reason for hiding this comment

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

The update is not complete, and has an issue in that it will still output a "classic" netcdf file.

I have a test case ready (see https://github.com/underwoo/FRE-NCtools/blob/test.and.source.cleanup/tests/mppnccombine/input-output-kinds). Please use this test case to update mppnccombine.

I also feel we should not limit the output to NetCDF 4 with Classic. We should support the full NetCDF 4 file (with HDF4/5).

@HansVahlenkamp
Copy link
Contributor Author

HansVahlenkamp commented Dec 10, 2024

The update is not complete, and has an issue in that it will still output a "classic" netcdf file.

I have a test case ready (see https://github.com/underwoo/FRE-NCtools/blob/test.and.source.cleanup/tests/mppnccombine/input-output-kinds). Please use this test case to update mppnccombine.

I also feel we should not limit the output to NetCDF 4 with Classic. We should support the full NetCDF 4 file (with HDF4/5).

Well, netCDF4 classic model format is still based on HDF5. The mppnccombine "-n4" option has always written out netCDF4 classic model format. That's why I did the same thing when the -n4 option is not used and the input is any netCDF4 to write out netCDF4 classic model format. Is that too restrictive? Are any GFDL codes writing out netCDF4 that is not classic model format?

@underwoo
Copy link
Member

Well, netCDF4 classic model format is still based on HDF5. The mppnccombine "-n4" option has always written out netCDF4 classic model format. That's why I did the same thing when the -n4 option is not used and the input is any netCDF4 to write out netCDF4 classic model format. Is that too restrictive? Are any GFDL codes writing out netCDF4 that is not classic model format?

Not yet, but some of the components are using HDF5 features in the NetCDF4 files. Since we are making the updates to mppnccombine now, we should future proof the output file instead of waiting.

As mentioned in the previous comment, the way the update is written now there is a bug in the logic. The test still had a "classic" netcdf file written out.

If the netCDF format for the output file is not chosen with the -64 or -n4 options then automatically use the netCDF format of the first input file; except if the first input file is netCDF classic then use netCDF 64-bit offset instead for the output file.
@HansVahlenkamp
Copy link
Contributor Author

Well, netCDF4 classic model format is still based on HDF5. The mppnccombine "-n4" option has always written out netCDF4 classic model format. That's why I did the same thing when the -n4 option is not used and the input is any netCDF4 to write out netCDF4 classic model format. Is that too restrictive? Are any GFDL codes writing out netCDF4 that is not classic model format?

Not yet, but some of the components are using HDF5 features in the NetCDF4 files. Since we are making the updates to mppnccombine now, we should future proof the output file instead of waiting.

As mentioned in the previous comment, the way the update is written now there is a bug in the logic. The test still had a "classic" netcdf file written out.

There was previously no test for the netCDF cdf5 format of an input file, but this is now included. There is also updated code to allow any netCDF format from the input file to be used for the output file; except if the input file is netCDF classic then netCDF 64-bit offset will be used instead. (The "-n4" option still specifies using netCDF4 classic model format.)

@mlee03 mlee03 merged commit 7b34d65 into main Dec 11, 2024
0 of 8 checks passed
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.

mppnccombine needs update to use the input format by default
4 participants