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

Are we using ESMF_TimeGet timeString in places we shouldn't? #3297

Open
mathomp4 opened this issue Jan 9, 2025 · 1 comment
Open

Are we using ESMF_TimeGet timeString in places we shouldn't? #3297

mathomp4 opened this issue Jan 9, 2025 · 1 comment
Assignees
Labels
⌛ Long Term Long term issues ❓ Question Further information is requested

Comments

@mathomp4
Copy link
Member

mathomp4 commented Jan 9, 2025

This is mainly for @bena-nasa to ponder. Today, he helped me try to figure out why my Spack CI tests here with MAPL were failing. Eventually, we figured out it was because the timeString bits of ESMF_TimeGet were returning garbage.

This was actually the @atrayano sprintf bug fixed by @billsacks in esmf-org/esmf#298. So when I finagled Spack to build ESMF 8.8.0 (which is always fun with a "not-quite-released" package 😄 ), all worked, see:

https://github.com/GEOS-ESM/MAPL/actions/runs/12697215473/job/35393142177?pr=3123

But, before @bena-nasa and I realized that, we actually figured out how to fix up bits of MAPL to let it work. And we saw some oddities. For example:

! Get Clock StartTime for Default ref_date, ref_time
! --------------------------------------------------
call ESMF_ClockGet ( clock, calendar=cal, _RC )
call ESMF_ClockGet ( clock, currTime=CurrTime, _RC )
call ESMF_ClockGet ( clock, StartTime=StartTime,_RC )
call ESMF_TimeGet ( StartTime, TimeString=string ,_RC )
read(string( 1: 4),'(i4.4)') year
read(string( 6: 7),'(i2.2)') month
read(string( 9:10),'(i2.2)') day
read(string(12:13),'(i2.2)') hour
read(string(15:16),'(i2.2)') minute
read(string(18:18),'(i2.2)') second
nymd0 = year*10000 + month*100 + day
nhms0 = hour*10000 + minute*100 + second
call ESMF_TimeGet ( CurrTime, TimeString=string ,_RC )
read(string( 1: 4),'(i4.4)') year
read(string( 6: 7),'(i2.2)') month
read(string( 9:10),'(i2.2)') day
read(string(12:13),'(i2.2)') hour
read(string(15:16),'(i2.2)') minute
read(string(18:18),'(i2.2)') second
nymdc = year*10000 + month*100 + day
nhmsc = hour*10000 + minute*100 + second

We realized that this could be simplfied to:

! Get Clock StartTime for Default ref_date, ref_time
! --------------------------------------------------
    call ESMF_ClockGet ( clock,     calendar=cal,       _RC )
    call ESMF_ClockGet ( clock,     currTime=CurrTime,  _RC )
    call ESMF_ClockGet ( clock,     StartTime=StartTime,_RC )
    call ESMF_TimeGet ( StartTime yy=year, mm=month, dd=day, h=hour, m=minute, s=second ,_RC )

    nymd0 =  year*10000 +  month*100 + day
    nhms0 =  hour*10000 + minute*100 + second

    call ESMF_TimeGet  ( CurrTime, yy=year, mm=month, dd=day, h=hour, m=minute, s=second, _RC )

    nymdc =  year*10000 +  month*100 + day
    nhmsc =  hour*10000 + minute*100 + second

So, I wonder if there are places timeString is being used that are unnecessary?

@mathomp4 mathomp4 added ❓ Question Further information is requested ⌛ Long Term Long term issues labels Jan 9, 2025
@darianboggs
Copy link
Contributor

I agree. We have good code in several places in MAPL that handles date and time information. Some of it uses ESMF, and some of it uses our own date and time code. We should consolidate it in one place. My experience is that code that parsing strings is fragile, and so it should only be used when necessary. Further, it's best to consolidate it one place with variations together using a common core code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⌛ Long Term Long term issues ❓ Question Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants