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

mppnccombine needs update to use the input format by default #319

Closed
ceblanton opened this issue Oct 22, 2024 · 19 comments · Fixed by #326
Closed

mppnccombine needs update to use the input format by default #319

ceblanton opened this issue Oct 22, 2024 · 19 comments · Fixed by #326
Assignees
@HansVahlenkamp
Labels
enhancement

Comments

@ceblanton
Copy link
Contributor

ceblanton commented Oct 22, 2024 •
edited
Loading

Describe the bug
mppnccombine supports Netcdf-3 classic, Netcdf-3 64-bit offset, and Netcdf-4 input file and output file formats.

The FRE team believed that if you specify the output format (i.e. -64 or -n4), then the tool will use the format. Otherwise, it should use the input format.

Unfortunately, this is not how the tool works. How it works is: If you specify the output format (-64 or -n4), then the tool will use that format (Netcdf-3 64-bit offset and Netcdf-4 classic). But if the output format is not specified, then the default is Netcdf-3 classic.

To Reproduce
Find some test input files and convert them so both Netcdf-4 and Netcdf-3 64-bit offset are available.

Run mppncombine without specifying the output format, and observe the behavior.

In both cases, the tool chooses the never-desired Netcdf-3 classic format.

(use ncdump -k to see the file type)

Expected behavior
When the input files are either Netcdf-3 64-bit offset or regular Netcdf-3, and no output format is specified, then the output format should be Netcdf-3 64-bit offset.

When the input files are Netcdf-4 and no output format is specified, then the output format should be Netcdf-4.

System Environment
any

Additional context
Add any other context about the problem. If applicable, include where any files
that help describe, or reproduce the problem exist.
@ceblanton
Copy link
Contributor Author

@HansVahlenkamp, as the original and most recent mppnccombine maintainer, could you fix this? Thank you!

@HansVahlenkamp HansVahlenkamp self-assigned this Oct 23, 2024
@mlee03
Copy link
Contributor

mlee03 commented Nov 14, 2024

Hi @HansVahlenkamp, any update on this issue? 😁

@HansVahlenkamp
Copy link
Contributor

Distracted with some other issues that needed attention. Will work on this next next week.

@mlee03
Copy link
Contributor

mlee03 commented Nov 14, 2024

@HansVahlenkamp, got it. I'll pester you next week then :)

@ceblanton
Copy link
Contributor Author

@HansVahlenkamp , when you update mppnccombine, please come up with 3 clear test cases to show that the fix is good.

@underwoo volunteered to convert your documented test cases to unit tests, but he needs the 3 clear input files to do this.

Thank you! (and PS, I'll be pestering you too! we really need this update next week)

Test case 1: Netcdf-3 (not 64-bit) input: output should be Netcdf-3 64-bit

Test case 2: Netcdf-3 (64-bit) input: output should be Netcdf-3 64-bit

Test case 3: Netcdf-4 input: Netcdf-4 output

@mlee03
Copy link
Contributor

mlee03 commented Nov 20, 2024

@HansVahlenkamp, just checking, any updates?

@HansVahlenkamp
Copy link
Contributor

@mlee03 I'm working on it this week and hope to have an updated version by next week...

Starting with version 2.2, there were significant structural changes done to the code by another author. The first step is reviewing those changes compared to the last version that I wrote. Also, my NetCDF test input files are quite old at this point and I could use some new ones (which are not too large) that represent all three of the test cases described by @ceblanton. Do you have any available?

@ceblanton
Copy link
Contributor Author

@HansVahlenkamp I'm sure your old test files are OK but I'll dig up some new ones.

@ceblanton
Copy link
Contributor Author

@HansVahlenkamp here are some distributed files on gaea, in the 3 formats:

gaea57:~%>ls /gpfs/f5/scratch/gfdl_f/Chris.Blanton/test-files/*                                                                                                                                                                                   1079
/gpfs/f5/scratch/gfdl_f/Chris.Blanton/test-files/nc3:
out.nc	vegn2.res.tile6.nc.0000  vegn2.res.tile6.nc.0001  vegn2.res.tile6.nc.0002  vegn2.res.tile6.nc.0003  vegn2.res.tile6.nc.0004

/gpfs/f5/scratch/gfdl_f/Chris.Blanton/test-files/nc3-64:
out.nc	vegn2.res.tile6.nc.0000  vegn2.res.tile6.nc.0001  vegn2.res.tile6.nc.0002  vegn2.res.tile6.nc.0003  vegn2.res.tile6.nc.0004

/gpfs/f5/scratch/gfdl_f/Chris.Blanton/test-files/nc4:
out.nc	vegn2.res.tile6.nc.0000  vegn2.res.tile6.nc.0001  vegn2.res.tile6.nc.0002  vegn2.res.tile6.nc.0003  vegn2.res.tile6.nc.0004

@mlee03
Copy link
Contributor

mlee03 commented Nov 25, 2024

Hi @HansVahlenkamp, any chance the PR can be made in the next couple of days?

@HansVahlenkamp
Copy link
Contributor

@mlee03 Hopefully done in the next couple of days.

@mlee03
Copy link
Contributor

mlee03 commented Dec 3, 2024

@HansVahlenkamp, I'm back! Any updates? Is it OK if you open up a draft PR as a placeholder?

@HansVahlenkamp
Copy link
Contributor

@mlee03 It turns out to be a bit more complicated than originally thought. The code creates the output file with a particular format before any of the input files are read. So, some code restructuring is needed to make this flexible based upon the format of the first input file. I'm currently testing some ideas for this restructuring. I was also investigating some other cleanup of the code, but that probably should wait until another time to minimize any further delays in resolving the main issue.

@mlee03
Copy link
Contributor

mlee03 commented Dec 3, 2024

That sounds like a lot of work. @ceblanton, is this PR still urgent?

@HansVahlenkamp
Copy link
Contributor

@mlee03 It's not too bad really; I can see what needs to be done. I'll make a separate PR at another time for non-essential cleanup of the code.

@mlee03
Copy link
Contributor

mlee03 commented Dec 3, 2024

Ok, sounds good then. Thank you Hans!

@HansVahlenkamp HansVahlenkamp linked a pull request Dec 5, 2024 that will close this issue
Update mppnccombine.c to V2.2.8 #326
Merged
@HansVahlenkamp
Copy link
Contributor

Discovered that the new mppnccombine causes one failure when running "make check":

...
FAIL: Test21-reference_mppnccombine.sh 1 reference mppnccombine combines comparison to bronx-16 stored copy
ERROR: Test21-reference_mppnccombine.sh - exited with status 1
...

It looks like Test21 needs to be updated to compensate for the new mppnccombine behavior.

@HansVahlenkamp
Copy link
Contributor

@HansVahlenkamp , when you update mppnccombine, please come up with 3 clear test cases to show that the fix is good.

@underwoo volunteered to convert your documented test cases to unit tests, but he needs the 3 clear input files to do this.

Thank you! (and PS, I'll be pestering you too! we really need this update next week)

Test case 1: Netcdf-3 (not 64-bit) input: output should be Netcdf-3 64-bit

Test case 2: Netcdf-3 (64-bit) input: output should be Netcdf-3 64-bit

Test case 3: Netcdf-4 input: Netcdf-4 output

The updated mppnccombine succeeds with all of those test input files. Do you have any more complex multi-dimensional input files for testing, particularly netCDF-4 using the classic model? The "Test case 3" input files are standard netCDF-4, not using the classic model. mppnccombine only outputs netCDF-4 using the classic model.

@underwoo
Copy link
Member

underwoo commented Dec 6, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@HansVahlenkamp HansVahlenkamp

Labels
enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Update mppnccombine.c to V2.2.8 NOAA-GFDL/FRE-NCtools
4 participants
@underwoo @HansVahlenkamp @ceblanton @mlee03