-
Notifications
You must be signed in to change notification settings - Fork 136
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_interp #1252
Conversation
@@ -30,7 +30,7 @@ program test_time_interp | |||
type(time_type) :: Time_beg, Time_end, Time(num_Time) | |||
type(time_type), allocatable, dimension(:) :: Timelist | |||
integer :: index1, index2, mo, yr, timelist_len, outunit, ntest, nline | |||
real :: weight | |||
real(TI_TEST_KIND_) :: weight |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like the test is a guide on how to use time_interp. Is there a way to check that the answers are as expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added checks for all but the last, since that one cycles through instead of going to set dates.
I think the coupler build will break with our mixed mode branch until we bring in the main libfms.F90 changes, but this should be ready now. Had to include the horiz_interp fix i made a pr for, otherwise data override's tests would break. |
@rem1776, could you compile with -Wconversion-extra with GCC? There seems to be one or two conversion mismatch |
@rem1776 all the time divides (time1//time2) return double precision values. I think they all need real() conversion. |
real(TI_TEST_KIND_) :: ref_weights(num_Time), ref_weights_leap(num_Time) | ||
real(TI_TEST_KIND_), parameter :: SMALL = 1.0e-7_kindl ! r4 will fail with 8 | ||
real(TI_TEST_KIND_), parameter :: midpoint = 0.483870967741935_kindl | ||
real(TI_TEST_KIND_), parameter :: day_before_leap_day = 0.964285714285714_kindl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where did these numbers come from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just reference weights from the calculation, its a fraction of time passed over a interval. So for this its the midway point of a month (jan 16). I think i commented on it a bit lower down.
I might be able to replace these with the actual time_type calculation if thats preferred, i guess i just like seeing what the actual value ends up as.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could the actual time_type calculation be used and add the numbers as comments? Is this perhaps the reason you're not getting exactly agreeing answers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After looking into it i don't think i can reproduce the calculation unless i copy in a whole routine (set_modtime). I think this way has some benefits though, like if the time_type arithmentic overrides return the wrong thing a check with the same operations will just be checking wrong answers against wrong answers.
@@ -123,6 +123,11 @@ do j=1, n3 | |||
enddo | |||
enddo | |||
|
|||
do j=1, n3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah it did that for you too! the do loops are repeated here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think something wonky might be happening with our branches, might be better to simplify things and just use mixedmode as the starting point going forward
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 <[email protected]> Co-authored-by: MiKyung Lee <[email protected]> Co-authored-by: mlee03 <[email protected]> Co-authored-by: Caitlyn McAllister <[email protected]> Co-authored-by: Jesse Lentz <[email protected]>
Description
Updates time_interp for mixed precision.
For time_interp, it just calculates fractional weights for different time units so most of the math is done in r8 (via operator overrides) for the time_type. The weight calculations are casted to the requested kind.
For time_interp_external2, it reads in files and interpolates with the calculated weight. Since the
init_external_field
routine does not have real arguments it always reads data in as r8. I tried to get around this by checking whats read in from fms2_io, but it'll only read in whatever type you give it for a buffer so will have to default to r8. The data is casted down for the calculations to match the kind of the weights. I also renamed thefield
module level list toloaded_fields
, avoids naming conflicts and is a bit more clear.The other change to note is in load_record (time_interp_external2). There is an optional argument to pass in a allocated horiz_interp_type which tells it to do a horizontal interpolation defined by horiz_interp_new. This causes a mismatch between the types in the two modules, so it casts down for the horiz_interp call if needed and copies the return values back over.
How Has This Been Tested?
latest intel on amd box.
Checklist:
make distcheck
passes