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

Enable PIO to Add Attributes to Existing NetCDF Files #1234

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

amstokely
Copy link
Contributor

@amstokely amstokely commented Sep 17, 2024

Add Ability to Write New Attributes to Existing NetCDF Files in PIO

Overview:

Currently, PIO in MPAS lacks the ability to add new attributes to existing NetCDF files. This limitation arises because when PIO attempts to write a new attribute, the file remains in data mode, which does not allow for the addition of attributes. Instead, attributes can only be added when the file is in define mode.

Unfortunately, the public APIs of both PIO and NetCDF do not provide a way to inquire about a file’s current mode. Switching to define mode each time a new attribute is needed is expensive, as transitioning between data and define modes introduces significant I/O costs.

@amstokely amstokely force-pushed the framework/pio_put_att branch from 228b58c to 7b75c62 Compare September 17, 2024 20:54
@mgduda
Copy link
Contributor

mgduda commented Oct 19, 2024

@amstokely Can you fix up the commit history? As of now it looks like there are 23 files changed.

- Adds support for adding new attributes to existing NetCDF files by
  minimizing expensive mode switches between data and define modes.
- Introduces `put_att_pio` interface with try-fail logic, handling scalar
  and 1D attributes of various data types (int, real, double, string).
- Enhances performance by avoiding unnecessary transitions and includes
  extensive logging for better traceability.
- Ensures backward compatibility for NetCDF files generated by earlier MPAS
  versions.
@amstokely amstokely force-pushed the framework/pio_put_att branch from 7b75c62 to c066590 Compare October 21, 2024 16:33
@amstokely
Copy link
Contributor Author

@amstokely Can you fix up the commit history? As of now it looks like there are 23 files changed.

@mgduda I just rebased off of the current MPAS-Dev develop branch. This should resolve the previously large diff.

@mgduda mgduda marked this pull request as ready for review November 1, 2024 20:14
@mgduda mgduda requested review from mgduda and jim-p-w November 1, 2024 20:15
src/framework/mpas_io.F Outdated Show resolved Hide resolved
src/framework/mpas_io.F Show resolved Hide resolved
src/framework/mpas_io.F Show resolved Hide resolved
src/framework/mpas_io.F Outdated Show resolved Hide resolved
src/framework/mpas_io.F Show resolved Hide resolved
if (handle_put_att_pio_enddef(handle) /= PIO_noerr) return
if (present(ierr)) ierr = MPAS_IO_NOERR
end if
return
Copy link
Contributor

Choose a reason for hiding this comment

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

We could remove this return statement.

src/framework/mpas_io.F Outdated Show resolved Hide resolved
@@ -5919,20 +6074,20 @@ subroutine MPAS_io_put_att_real1d(handle, attName, attValue, fieldname, syncVal,
allocate(singleVal(size(attValueLocal)))
singleVal(:) = real(attValueLocal(:),R4KIND)
#ifdef MPAS_PIO_SUPPORT
pio_ierr = PIO_put_att(handle % pio_file, varid, attName, singleVal)
pio_ierr = put_att_pio(handle, varid, attName, singleVal, ierr=ierr)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there may be another space of indentation that's needed here to align with other code.

@@ -5950,6 +6105,9 @@ subroutine MPAS_io_put_att_real1d(handle, attName, attValue, fieldname, syncVal,
end subroutine MPAS_io_put_att_real1d





Copy link
Contributor

Choose a reason for hiding this comment

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

It would be preferable to not add these three blank lines.

src/framework/mpas_io.F Show resolved Hide resolved
@@ -5033,6 +5037,149 @@ subroutine MPAS_io_get_att_real1d(handle, attName, attValue, fieldname, precisio

end subroutine MPAS_io_get_att_real1d

function handle_put_att_pio_redef(handle) result (pio_ierr)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is _put_att_ in these function names? They don't do anything with attributes.

src/framework/mpas_io.F Outdated Show resolved Hide resolved
src/framework/mpas_io.F Outdated Show resolved Hide resolved
src/framework/mpas_io.F Show resolved Hide resolved
@mgduda
Copy link
Contributor

mgduda commented Dec 9, 2024

@amstokely I've un-resolved several comments that were marked as resolved but didn't appear to actually have been addressed. Can you take another look at these?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants