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

Enable PIO to Add Attributes to Existing NetCDF Files #1234

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Enable PIO to Add Attributes to Existing NetCDF Files #1234

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
212 changes: 167 additions & 45 deletions src/framework/mpas_io.F
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,10 @@ module mpas_io

#ifdef MPAS_PIO_SUPPORT
integer, private :: io_global_err = PIO_noerr
interface put_att_pio
module procedure put_att_0d_generic_pio
module procedure put_att_1d_generic_pio
end interface put_att_pio
#endif
#ifdef MPAS_SMIOL_SUPPORT
integer, private :: io_global_err = SMIOL_SUCCESS
Expand Down Expand Up @@ -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.

If these new routines are only used by PIO, can we enclose them in an MPAS_PIO_SUPPORT pre-processing directive?

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.

implicit none
type(MPAS_IO_Handle_type), intent(inout) :: handle
integer :: pio_ierr

call mpas_log_write('Calling PIO_redef')
pio_ierr = PIO_redef(handle % pio_file)
if (pio_ierr /= PIO_noerr) then
io_global_err = pio_ierr
return
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this return statement is needed (except to avoid printing out the log message below).

end if
call mpas_log_write('Successfully called PIO_redef')
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's either remove or comment-out these logging statements.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should this function clear handle%data_mode? on success?


end function handle_put_att_pio_redef

function handle_put_att_pio_enddef(handle) result (pio_ierr)
implicit none
type(MPAS_IO_Handle_type), intent(inout) :: handle
integer :: pio_ierr

call mpas_log_write('Calling PIO_enddef')
pio_ierr = PIO_enddef(handle % pio_file)
if (pio_ierr /= PIO_noerr) then
io_global_err = pio_ierr
return
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to the comment earlier, if we remove the print statements, then this return statement can probably be removed as well.

end if
call mpas_log_write('Successfully called PIO_enddef')
Copy link
Contributor

Choose a reason for hiding this comment

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

should this function set handle%data_mode on success?


end function handle_put_att_pio_enddef

function put_att_0d_generic_pio(handle, varid, attName, attValue, ierr) result(pio_ierr)
implicit none
type(MPAS_IO_Handle_type), intent(inout) :: handle
integer, intent(in) :: varid
character(len=*), intent(in) :: attName
class(*), intent(in) :: attValue
integer, optional :: ierr
integer :: pio_ierr
character(len=*), parameter :: log_message_prefix = 'Calling PIO_put_att for'

select type(attValue)
type is (integer)
call mpas_log_write(log_message_prefix//' integer attribute '//trim(attname))
pio_ierr = PIO_put_att(handle % pio_file, varid, attName, attValue)
type is (real(kind=R4KIND))
pio_ierr = PIO_put_att(handle % pio_file, varid, attName, attValue)
call mpas_log_write(log_message_prefix//' real(kind=R4KIND) attribute '//trim(attname))
type is (real(kind=R8KIND))
call mpas_log_write(log_message_prefix//' real(kind=R8KIND) attribute '//trim(attname))
pio_ierr = PIO_put_att(handle % pio_file, varid, attName, attValue)
type is (character(len=*))
call mpas_log_write(log_message_prefix//' text attribute '//trim(attname))
pio_ierr = PIO_put_att(handle % pio_file, varid, attName, attValue)
end select

if (pio_ierr /= PIO_noerr) then
io_global_err = pio_ierr
if (present(ierr)) ierr = MPAS_IO_ERR_BACKEND

if (handle % preexisting_file .and. .not. handle % data_mode) then
if (handle_put_att_pio_redef(handle) /= PIO_noerr) return
Comment on lines +5099 to +5100
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 I don't understand what data_mode means. I assumed it meant the file is in data mode, in which case you can do data i/o, but can't create new attributes. If so, this logic seems incorrect, because if the file is not in data mode it must be in define mode, so setting the file state to define mode is redundant.

Furthermore, all of the call sites to put_att_pio check for handle%data_mode and return before doing any work if the file is in data mode. See lines 5376, 5549, and 5719.

What am I missing?

Copy link
Contributor

Choose a reason for hiding this comment

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

It sounds like we think the value of data_modemay be incorrect (that is, it isn't set even though the file is in data mode. If that is the case, line 5099 should not check the value of data_mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes this seems to be the cause of this problem. I'm looking into all the points you brought above.


select type(attValue)
type is (integer)
call mpas_log_write('Calling PIO_put_att for integer attribute '//trim(attname))
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove these logging statements as well.

pio_ierr = PIO_put_att(handle % pio_file, varid, attName, attValue)
type is (real(kind=R4KIND))
call mpas_log_write('Calling PIO_put_att for real(kind=R4KIND) attribute '//trim(attname))
pio_ierr = PIO_put_att(handle % pio_file, varid, attName, attValue)
type is (real(kind=R8KIND))
call mpas_log_write('Calling PIO_put_att for real(kind=R8KIND) attribute '//trim(attname))
pio_ierr = PIO_put_att(handle % pio_file, varid, attName, attValue)
type is (character(len=*))
call mpas_log_write('Calling PIO_put_att for text attribute '//trim(attname))
pio_ierr = PIO_put_att(handle % pio_file, varid, attName, attValue)
end select

if (pio_ierr /= PIO_noerr) then
io_global_err = pio_ierr
return
end if

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.

It doesn't look like this return statement is necessary.

end if
end function put_att_0d_generic_pio

function put_att_1d_generic_pio(handle, varid, attName, attValue, ierr) result(pio_ierr)
implicit none
type(MPAS_IO_Handle_type), intent(inout) :: handle
integer, intent(in) :: varid
character(len=*), intent(in) :: attName
class(*), dimension(:), intent(in) :: attValue
integer, optional :: ierr
integer :: pio_ierr
character(len=*), parameter :: log_message_prefix = 'Calling PIO_put_att for'

select type(attValue)
type is (integer)
call mpas_log_write(log_message_prefix//' integer 1D-array attribute '//trim(attname))
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove these logging statements as well.

pio_ierr = PIO_put_att(handle % pio_file, varid, attName, attValue)
type is (real(kind=R4KIND))
pio_ierr = PIO_put_att(handle % pio_file, varid, attName, attValue)
call mpas_log_write(log_message_prefix//' real(kind=R4KIND) 1D-array attribute '//trim(attname))
type is (real(kind=R8KIND))
call mpas_log_write(log_message_prefix//' real(kind=R8KIND) 1D-array attribute '//trim(attname))
pio_ierr = PIO_put_att(handle % pio_file, varid, attName, attValue)
end select

if (pio_ierr /= PIO_noerr) then
io_global_err = pio_ierr
if (present(ierr)) ierr = MPAS_IO_ERR_BACKEND

if (handle % preexisting_file .and. .not. handle % data_mode) then
if (handle_put_att_pio_redef(handle) /= PIO_noerr) return
select type(attValue)
type is (integer)
call mpas_log_write('Calling PIO_put_att for integer 1D-array attribute '//trim(attname))
pio_ierr = PIO_put_att(handle % pio_file, varid, attName, attValue)
type is (real(kind=R4KIND))
call mpas_log_write('Calling PIO_put_att for real(kind=R4KIND) 1D-array attribute '//trim(attname))
pio_ierr = PIO_put_att(handle % pio_file, varid, attName, attValue)
type is (real(kind=R8KIND))
call mpas_log_write('Calling PIO_put_att for real(kind=R8KIND) 1D-array attribute '//trim(attname))
pio_ierr = PIO_put_att(handle % pio_file, varid, attName, attValue)
end select

if (pio_ierr /= PIO_noerr) then
io_global_err = pio_ierr
return
end if

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.

end if
end function put_att_1d_generic_pio



subroutine MPAS_io_get_att_text(handle, attName, attValue, fieldname, ierr)

Expand Down Expand Up @@ -5338,7 +5485,7 @@ subroutine MPAS_io_put_att_int0d(handle, attName, attValue, fieldname, syncVal,
end if

#ifdef MPAS_PIO_SUPPORT
pio_ierr = PIO_put_att(handle % pio_file, varid, attName, attValueLocal)
pio_ierr = put_att_pio(handle, varid, attName, attValueLocal, ierr=ierr)
if (pio_ierr /= PIO_noerr) then
io_global_err = pio_ierr
if (present(ierr)) ierr = MPAS_IO_ERR_BACKEND
Expand Down Expand Up @@ -5523,7 +5670,7 @@ subroutine MPAS_io_put_att_int1d(handle, attName, attValue, fieldname, syncVal,
end if

#ifdef MPAS_PIO_SUPPORT
pio_ierr = PIO_put_att(handle % pio_file, varid, attName, attValueLocal)
pio_ierr = put_att_pio(handle, varid, attName, attValueLocal, ierr=ierr)
if (pio_ierr /= PIO_noerr) then
io_global_err = pio_ierr
if (present(ierr)) ierr = MPAS_IO_ERR_BACKEND
Expand Down Expand Up @@ -5689,7 +5836,7 @@ subroutine MPAS_io_put_att_real0d(handle, attName, attValue, fieldname, syncVal,
(MPAS_IO_NATIVE_PRECISION /= MPAS_IO_SINGLE_PRECISION)) then
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)
#endif

#ifdef MPAS_SMIOL_SUPPORT
Expand All @@ -5703,7 +5850,7 @@ subroutine MPAS_io_put_att_real0d(handle, attName, attValue, fieldname, syncVal,
(MPAS_IO_NATIVE_PRECISION /= MPAS_IO_DOUBLE_PRECISION)) then
doubleVal = real(attValueLocal,R8KIND)
#ifdef MPAS_PIO_SUPPORT
pio_ierr = PIO_put_att(handle % pio_file, varid, attName, doubleVal)
pio_ierr = put_att_pio(handle, varid, attName, doubleVal, ierr=ierr)
#endif

#ifdef MPAS_SMIOL_SUPPORT
Expand All @@ -5715,7 +5862,7 @@ subroutine MPAS_io_put_att_real0d(handle, attName, attValue, fieldname, syncVal,
#endif
else
#ifdef MPAS_PIO_SUPPORT
pio_ierr = PIO_put_att(handle % pio_file, varid, attName, attValueLocal)
pio_ierr = put_att_pio(handle, varid, attName, attValueLocal, ierr=ierr)
#endif

#ifdef MPAS_SMIOL_SUPPORT
Expand All @@ -5733,6 +5880,14 @@ subroutine MPAS_io_put_att_real0d(handle, attName, attValue, fieldname, syncVal,
if (present(ierr)) ierr = MPAS_IO_ERR_BACKEND
return
end if

! if (handle % preexisting_file) then
! pio_ierr = PIO_enddef(handle % pio_file)
! if (pio_ierr /= PIO_noerr) then
! io_global_err = pio_ierr
! return
! end if
! end if
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps it's better to just remove this block of code rather than leaving it commented-out?

#endif
#ifdef MPAS_SMIOL_SUPPORT
if (local_ierr /= SMIOL_SUCCESS) then
Expand Down Expand Up @@ -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.

#endif
deallocate(singleVal)
else if ((new_attlist_node % attHandle % precision == MPAS_IO_DOUBLE_PRECISION) .and. &
(MPAS_IO_NATIVE_PRECISION /= MPAS_IO_DOUBLE_PRECISION)) then
allocate(doubleVal(size(attValueLocal)))
doubleVal(:) = real(attValueLocal(:),R8KIND)
#ifdef MPAS_PIO_SUPPORT
pio_ierr = PIO_put_att(handle % pio_file, varid, attName, doubleVal)
pio_ierr = put_att_pio(handle, varid, attName, doubleVal, ierr=ierr)
#endif
deallocate(doubleVal)
else
#ifdef MPAS_PIO_SUPPORT
pio_ierr = PIO_put_att(handle % pio_file, varid, attName, attValueLocal)
pio_ierr = put_att_pio(handle, varid, attName, attValueLocal, ierr=ierr)
#endif
end if
#ifdef MPAS_PIO_SUPPORT
Expand All @@ -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.

subroutine MPAS_io_put_att_text(handle, attName, attValue, fieldname, syncVal, ierr)

implicit none
Expand Down Expand Up @@ -6100,43 +6258,7 @@ subroutine MPAS_io_put_att_text(handle, attName, attValue, fieldname, syncVal, i
end if

#ifdef MPAS_PIO_SUPPORT
pio_ierr = PIO_put_att(handle % pio_file, varid, attName, trim(attValueLocal))
if (pio_ierr /= PIO_noerr) then

io_global_err = pio_ierr
if (present(ierr)) ierr = MPAS_IO_ERR_BACKEND

!
! If we are working with a pre-existing file and the text attribute is larger than in the file, we need
! to enter define mode before writing the attribute. Note the PIO_redef documentation:
! 'Entering and leaving netcdf define mode causes a file sync operation to occur,
! these operations can be very expensive in parallel systems.'
!
if (handle % preexisting_file .and. .not. handle % data_mode) then
pio_ierr = PIO_redef(handle % pio_file)
if (pio_ierr /= PIO_noerr) then
io_global_err = pio_ierr
return
end if

pio_ierr = PIO_put_att(handle % pio_file, varid, attName, trim(attValueLocal))
if (pio_ierr /= PIO_noerr) then
io_global_err = pio_ierr
return
end if

pio_ierr = PIO_enddef(handle % pio_file)
if (pio_ierr /= PIO_noerr) then
io_global_err = pio_ierr
return
end if

if (present(ierr)) ierr = MPAS_IO_NOERR

end if

return
end if
pio_ierr = put_att_pio(handle, varid, attName, attValueLocal, ierr=ierr)
#endif

#ifdef MPAS_SMIOL_SUPPORT
Expand Down