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

MAPL_Config bug with GNU and GOCART2G #951

Closed
mathomp4 opened this issue Aug 9, 2021 · 10 comments
Closed

MAPL_Config bug with GNU and GOCART2G #951

mathomp4 opened this issue Aug 9, 2021 · 10 comments
Assignees
Labels
🪲 Bugfix This fixes a bug!

Comments

@mathomp4
Copy link
Member

mathomp4 commented Aug 9, 2021

I was trying to run GOCART2G with GNU and I found a bug in MAPL. To wit, when you run you get:

At line 659 of file /discover/swdev/mathomp4/Models/GEOSgcm-feature-geos-v10.17.6-gocart2g-v2.0.0-GNU/GEOSgcm/src/Shared/@MAPL/base/MAPL_Config.F90
Fortran runtime error: End of record

which points to MAPL_ConfigSetAttribute_reals32 a function added by @atrayano:

MAPL/base/MAPL_Config.F90

Lines 649 to 664 in d500930

integer, parameter :: LSZ = max (1024,ESMF_MAXPATHLEN) ! Maximum line size
character(len=LSZ) :: buffer
character(len=15) :: tmpStr, newVal
integer :: count, i, j
integer :: status
count = size(value)
buffer = '' ! initialize to
do i = 1, count
j = len_trim(buffer)
write(tmpStr, *) value(i) ! ALT: check if enough space to write
newVal = adjustl(tmpStr)
_ASSERT(j + len_trim(newVal) < LSZ,'not enough space to write')
write(buffer(j+1:), *) trim(newVal)
end do
call MAPL_ConfigSetAttribute(config, value=buffer, label=label, __RC__)

The issue seems to be with the 'magic number' 15. If you make a small little Fortran program:

program main

   implicit none

   character(len=15) :: tmpStr
   real :: value(5)
   integer :: i

   value = [500, 600, 700, 800, 900]

   do i = 1, size(value)
      write(*,*) "i: ", i, "value: ", value(i)
      write(tmpStr,*) value(i)
      write(*,*) 'tmpStr:', tmpStr
   end do

end program main

This works with Intel:

ifort test.F90 && ./a.out
 i:            1 value:    500.0000
 tmpStr:   500.0000
 i:            2 value:    600.0000
 tmpStr:   600.0000
 i:            3 value:    700.0000
 tmpStr:   700.0000
 i:            4 value:    800.0000
 tmpStr:   800.0000
 i:            5 value:    900.0000
 tmpStr:   900.0000

but fails with GNU:

gfortran test.F90 && ./a.out
 i:            1 value:    500.000000
At line 26 of file test.F90
Fortran runtime error: End of record

So there are a couple of things. One, it looks like Intel is doing F11.4 so we could change:

write(tmpStr, *) value(i) ! ALT: check if enough space to write

to:

        write(tmpStr, '(F11.4)') value(i) ! ALT: check if enough space to write

If so, we'd also probably want to look at the magic number in the int-vector case:

character(len=12) :: tmpStr, newVal

and maybe do:

        write(tmpStr, '(I0)') value(i) ! ALT: check if enough space to write

??

Perhaps also, we should have unit tests for this in base/tests/Test_MAPL_Config.pf which only has a test for string:

@test
subroutine test_SetAttribute_string

Not sure. @tclune might wish to weigh in.

@mathomp4 mathomp4 added the 🪲 Bugfix This fixes a bug! label Aug 9, 2021
@stale
Copy link

stale bot commented Oct 8, 2021

This issue has been automatically marked as stale because it has not had recent activity. If there are no updates within 7 days, it will be closed. You can add the "long term" tag to prevent the Stale bot from closing this issue.

@stale stale bot added the ❄️ Stale This issue has been marked stale label Oct 8, 2021
@stale
Copy link

stale bot commented Oct 15, 2021

Closing due to inactivity

@stale stale bot closed this as completed Oct 15, 2021
@atrayano atrayano reopened this Oct 15, 2021
@stale stale bot removed the ❄️ Stale This issue has been marked stale label Oct 15, 2021
@atrayano
Copy link
Contributor

This issue is not yet resolved ...

@tclune
Copy link
Collaborator

tclune commented Oct 15, 2021

Can't we just make the buffer len=32 and be done with this?

@atrayano
Copy link
Contributor

@tclune Yes we can, but this would limit the size of the "vector" to size(buffer)/32. I was trying use the least amount to have more possible entries... We can talk about it on Monday

@atrayano
Copy link
Contributor

If len=16 works for both compilers, this would be my preferred choice. We are trying to phase out the ESMF_Config, and there is no good reason to make it perfect just before before we switch to YAML

@mathomp4
Copy link
Member Author

Note, I did some testing with Atanas and it turns out the magic numbers are:

  • Intel: 15
  • NAG: 16
  • NVHPC: 16
  • GNU: 18

18 is ugly, but GNU is weird?

@mathomp4
Copy link
Member Author

mathomp4 commented Nov 1, 2021

@atrayano Are you okay with this being 18 characters as that's the GNU magic number?

@tclune
Copy link
Collaborator

tclune commented Nov 1, 2021

If not, I can work with you to convert this to a deferred length string where we explicitly allocate to a length long enough to contain whatever needs to go in.

atrayano added a commit that referenced this issue Nov 1, 2021
…internal_write_size

Fixes #951. Adjusted the size for the internal write buffer
@atrayano atrayano closed this as completed Nov 2, 2021
@atrayano
Copy link
Contributor

atrayano commented Nov 2, 2021

Fixed in #1164

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🪲 Bugfix This fixes a bug!
Projects
None yet
Development

No branches or pull requests

3 participants