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

mixed precision: time_manager reals to r8 and clean up #1196

Merged
merged 15 commits into from
Jun 7, 2023

Conversation

rem1776
Copy link
Contributor

@rem1776 rem1776 commented Apr 19, 2023

Description
replaces default reals and double precision values with explicit r8_kinds.

i created wrappers for get_cal_time and real_to_time_type so that they convert passed in r4's to r8 before calling the full routines. This makes all the real calculations done in r8_kind instead of default real precision.

I also cleaned up a lot of the old html comments since some were left in this module and made a small update to the test.

How Has This Been Tested?
oneapi 23.0, amd

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • Any dependent changes have been merged and published in downstream modules
  • New check tests, if applicable, are included
  • make distcheck passes


if(.not.module_is_initialized) call time_manager_init

! Convert time intervals to floating point days; risky for general performance?
d1 = time1%days * dble(seconds_per_day) + dble(time1%seconds) + time1%ticks/dble(ticks_per_second)
d2 = time2%days * dble(seconds_per_day) + dble(time2%seconds) + time2%ticks/dble(ticks_per_second)
d1 = time1%days * real(seconds_per_day, r8_kind) + real(time1%seconds, r8_kind) &
Copy link
Contributor

Choose a reason for hiding this comment

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

real(time1%days, r8_kind)

Copy link
Contributor

Choose a reason for hiding this comment

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

@rem1776 this change :)

d2 = time2%days * dble(seconds_per_day) + dble(time2%seconds) + time2%ticks/dble(ticks_per_second)
d1 = time1%days * real(seconds_per_day, r8_kind) + real(time1%seconds, r8_kind) &
+ time1%ticks/real(ticks_per_second, r8_kind)
d2 = time2%days * real(seconds_per_day, r8_kind) + real(time2%seconds, r8_kind) &
Copy link
Contributor

Choose a reason for hiding this comment

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

real(time2%days, r8_kind)

Copy link
Contributor

Choose a reason for hiding this comment

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

@rem1776 this one too

end function real_to_time_type
end function real8_to_time_type

function real4_to_time_type(x, err_msg) result(t)
Copy link
Contributor

Choose a reason for hiding this comment

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

would this function be needed if all reals are r8_kind in time_manager?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it's for external routines, if a module is compiled with r4 it'll call this to get a new time type. It gets converted/rounded anyway by safe_rtoi

div = d/dble(n)
dseconds_per_day = real(seconds_per_day, r8_kind)
dticks_per_second = real(ticks_per_second, r8_kind)
d = time%days*dseconds_per_day*dticks_per_second + real(time%seconds, r8_kind)*dticks_per_second + &
Copy link
Contributor

Choose a reason for hiding this comment

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

real(time%days,r8_kind)

Copy link
Contributor

Choose a reason for hiding this comment

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

@rem1776 this one too

dticks_per_second = real(ticks_per_second, r8_kind)
d = time%days*dseconds_per_day*dticks_per_second + real(time%seconds, r8_kind)*dticks_per_second + &
real(time%ticks, r8_kind)
div = d/real(n, r8_kind)

days = int(div/(dseconds_per_day*dticks_per_second))
seconds = int(div/dticks_per_second - days*dseconds_per_day)
Copy link
Contributor

Choose a reason for hiding this comment

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

real(days,r8_kind)

Copy link
Contributor

Choose a reason for hiding this comment

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

and here

@@ -2528,7 +2049,7 @@ function increment_date_private(Time, years, months, days, hours, minutes, secon
cmonth = cmonth + months

! Adjust year and month number when cmonth falls outside the range 1 to 12
cyear = cyear + floor((cmonth-1)/12.)
cyear = cyear + floor((cmonth-1)/12.0_r8_kind)
Copy link
Contributor

Choose a reason for hiding this comment

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

real(cmonth-1,r8_kind)

@@ -313,34 +322,50 @@ function get_cal_time(time_increment, units, calendar, permit_calendar_conversio
increment_days = floor(dt/86400)
increment_seconds = int(dt - increment_days*86400)
else
Copy link
Contributor

Choose a reason for hiding this comment

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

lines 292 -323, time_increment and dt are reals doing math with ints

!------------------------------------------------------------------------

!> For mixed precision support, just casts to passed in increment to r8
function get_calendar_time_wrap(time_increment, units, calendar, permit_calendar_conversion)
Copy link
Contributor

Choose a reason for hiding this comment

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

wondering if this function is needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was needed to compile with current mixed mode so I think it'll be needed

Copy link
Contributor

Choose a reason for hiding this comment

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

do you think using class(*) for time_increment would work? Or is that going to make things messier..

@mlee03
Copy link
Contributor

mlee03 commented Apr 20, 2023

@rem1776 could you also add mixed precision to the pr title? :)

@rem1776 rem1776 changed the title time_manager reals to r8 and clean up mixed precision: time_manager reals to r8 and clean up Apr 20, 2023
@mlee03
Copy link
Contributor

mlee03 commented May 31, 2023

@rem1776 ignore recent comments, did not see that they were outdated!

@mlee03
Copy link
Contributor

mlee03 commented Jun 5, 2023

@J-Lentz @uramirez @mcallic2 don't forget to review this one

if (rval .le. big .and. -1.*rval .ge. -1.*big) then
real(r8_kind) :: big
big = real(huge(ival), r8_kind)
if (rval .le. big .and. -1.0_r8_kind*rval .ge. -1.0_r8_kind*big) then
Copy link
Contributor

Choose a reason for hiding this comment

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

if (rval .le. big .and. -1.0_r8_kind*rval .ge. -1.0_r8_kind*big) then

I'm confused by this if condition. Under what circumstance would the comparisons on the left and right of the .and. ever differ from one another?

@rem1776 rem1776 merged commit f655759 into NOAA-GFDL:mixedmode Jun 7, 2023
rem1776 added a commit that referenced this pull request Sep 20, 2023
BREAKING CHANGE: In coupler_types.F90,  `coupler_nd_field_type` and `coupler_nd_values_type` have been renamed to indicate real kind value: `coupler_nd_real4/8_field_type` and `coupler_nd_real4/8_values_type`. The `bc` field within `coupler_nd_bc_type` was modified to use r8_kind within the value and field types, and an additional field added `bc_r4` to use r4_kind values.

Includes:

* feat: eliminate use of real numbers for mixed precision in `block_control` (#1195)

* feat: mixed precision field_manager  (#1205)

* feat: mixed precision random_numbers_mod (#1191)

* feat: mixed precision time_manager reals to r8 and clean up (#1196)

* feat: mixed Precision tracer_manager  (#1212)

* Mixed precision monin_obukhov (#1116)

* Mixed precision: `monin_obukhov` unit tests (#1272)

* mixed-precision diag_integral_mod  (#1217)

* mixed precision time_interp (#1252)

* mixed precision interpolator_mod  (#1305)

* Mixed precision astronomy (#1092)

* Mixed precision `data_override_mod` (#1323)

* mixed precision exchange (#1341)

* coupler mixed precision (#1353)

* Mixed precision topography_mod (#1250)

---------

Co-authored-by: rem1776 <Ryan.Mulhall@noaa.gov>
Co-authored-by: MiKyung Lee <58964324+mlee03@users.noreply.github.com>
Co-authored-by: mlee03 <Mikyung.Lee@lscamd50-d.gfdl.noaa.gov>
Co-authored-by: Caitlyn McAllister <65364559+mcallic2@users.noreply.github.com>
Co-authored-by: Jesse Lentz <42011922+J-Lentz@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants