Provide consistency in setting of prevRingTime for alarms when changing ref time #1290
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mgduda
force-pushed
the
framework/adjust_alarm_prevRingTime
branch
from
02292ee to
612799b
Compare
This commit adds unit tests for the mpas_adjust_alarm_to_reference_time routine to the test_core_timekeeping_tests module in the test core. Implied in these tests is the requirement that, after a call to mpas_adjust_alarm_to_reference_time, the prevRingTime of the alarm is set to a value that ensures that an immediately subsequent call to mpas_is_alarm_ringing will show that the alarm is ringing. In order to get more verbose printout, including the internal state of alarms, the MPAS_ADJUST_ALARM_VERBOSE macro can be modfied near the top of the test_core_timekeeping_tests module so that its argument is not turned into a comment.
mgduda
force-pushed
the
framework/adjust_alarm_prevRingTime
branch
from
612799b to
f6039c4
Compare
|
@abishekg7 @gdicker1 For more background, the fix in this PR to the |
|
When reviewing the changes to the |
|
After noodling this around a bit, the changes make sense. I think the message for the commit modifying mpas_adjust_alarm_to_reference_time could use some work. I think it would at least be nice if the newly consistent behavior was added to the first paragraph, even if the terms will be defined or expanded on more later. Though, I'm having trouble phrasing it (maybe not understanding as well as I'd like). Could be something like:
|
abishekg7
reviewed
May 20, 2025
|
The changes to |
How about the following for the first part of the commit message? |
|
@mgduda, that looks great! |
gdicker1
approved these changes
May 20, 2025
abishekg7
approved these changes
May 20, 2025
…ng ref time
This commit aims to provide consistency in the ringing behavior of recurring
alarms after their reference time has been adjusted with a call to
mpas_adjust_alarm_to_reference_time. Now, adjusting the reference time for an
alarm will always leave that alarm in a state such that it is considered by the
mpas_is_alarm_ringing routine to be ringing at the current time.
For simplicity of discussion, consider only a clock that is running forward in
time. The objective of the mpas_adjust_alarm_to_reference_time routine is to set
the prevRingTime (previous ring time) of a recurring alarm to (1) a time that
differs from the specified reference time by an integer multiple of the alarm
interval; and (2) the latest such time that is not after the current time on the
clock.
Prior to this commit, the logic in the mpas_adjust_alarm_to_reference_time
routine resulted in one of two outcomes:
(1) If the difference between the reference time and the current time is
divisible by the alarm interval, the prevRingTime becomes the current time, as
illustrated below.
ring ring ring ring
v v v v
-----------------------------------------------------------
^
now
prevRingTime
(2) Otherwise, the prevRingTime becomes the latest time before the current time
that lies on an integer multiple of the alarm interval away from the reference
time, as illustrated below.
ring ring ring ring
v v v v
-----------------------------------------------------------
^ ^
prevRingTime now
To determine whether a recurring alarm is ringing, the alarm's interval is added
to the alarm's prevRingTime. If the result is on or before the current time, the
alarm is ringing; otherwise, if the result is later than the current time, the
alarm is not ringing. As a consequence, outcome (1) from the
mpas_adjust_alarm_to_reference_time leads to an alarm that is ringing after the
call to the routine, while outcome(2) leads to an alarm that is not yet ringing.
In order to rectify the inconsistency in whether an alarm is ringing depending
on where its reference time is set relative to the current time, the
prevRingTime for an alarm is always set to be at least one full alarm interval
before the current time. Whether the difference between the current time and the
reference time is evenly divisible by the alarm's ring interval or not, a query
of the alarm's status immediately following the call to
mpas_adjust_alarm_to_reference_time will always show that it is ringing.
This commit makes changes to the logic for both a forward and a backward running
clock in the mpas_adjust_alarm_to_reference_time routine, although it appears
that no code actually makes use of a backward running clock at present.
Note: At present, the logic is not quite as elegant as one might expect it
should be. In future, if the interval_division routine handled negative
intervals like the analog of
(-2.75 % 1.0) => 0.25
then there may be no need for if-tests within blocks for each clock direction.
mgduda
force-pushed
the
framework/adjust_alarm_prevRingTime
branch
from
f6039c4 to
66d310a
Compare
I've updated the commit message as well as the PR description. |
mgduda
added a commit
that referenced
this pull request
May 23, 2025
This merge addresses several issues in the MPAS-Atmosphere model and in the MPAS infrastructure. Specific changes include: * Correction of the pool from which lbc_scalar constituent indices are obtained in the init_atm_thompson_aerosols_lbc routine. Rather than obtaining index_nifa and index_nwfa from the state pool, the indices of lbc_nifa and lbc_nwfa should be obtained from the lbc_state pool. (PR #1249) * Correction to the computation of the soil temperature (TSLB) in the Noah-MP land surface scheme through the addition of initialization of the soil liquid water (SH2O) in the noahmp_init subroutine in module mpas_atmphys_lsm_noahmpinit.F prior to calling NoahmpInitMain. (PR #1244) * Correction of the units of the fields 'greenfrac', 'shdmin', 'shdmax', 'vegfra', and 'albedo12m' from "unitless" to "percent" in the init_atmosphere and atmosphere core Registry.xml files. Also, a correction to the spelling of 'greenness' in several places. (PR #1248) * Removal of a duplicate allocation of indexToEdgeID % array in the mpas_io_setup_edge_block_fields routine that was the source of a memory leak. (PR #1258) * Fix for a memory leak in mpas_block_creator_build_cell_halos by deallocating the cellLimitField field before the routine returns. (PR #1264) * Fix for a bug in the logic for determining when decompositions can be reused by the SMIOL library. In almost any practical situation, however, this bug created no issues. (PR #1288) * Changes in the init_atmosphere core to provide more reliable error messages in case config_nfglevels is not set to a value that is at least as large as the number of vertical levels in the first-guess intermediate file. (PR #1291) * Correction of the loop for Noah-MP snow initialization, capping snow water equivalent maximum at 2000 mm. (PR #1300) * Fix for a bug in the horizontal 2nd-order filter for the CAM upper absorbing layer, where the wrong level in the kdiff field was being used when enforcing a lower-bound on kdiff. This absorbing layer is active only when config_mpas_cam_coef > 0.0. (PR #1302) * Fix in the mountain wave idealized test case initialization when multiple MPI tasks are used. The 'xc' variable, which represents the center-point location of the mountain, was previously computed based on the maximum xCell values local to an MPI task, leading to inconsistent values on each MPI rank. By finding the maximum of xCell over all MPI ranks and ensuring that all MPI ranks use this global maximum, the terrain field is computed consistently between serial and parallel runs of the init_atmosphere_model program for the mountain wave test case (config_init_case = 6). (PR #1312) * Correction to the calculation of the 2-meter diagnostics (T2M, TH2M, and Q2) when using the Noah-MP land surface scheme. While the computation of 2-meter diagnostics is the same for Noah and Noah-MP over oceans, it is different between the two land surface schemes over land. In Noah-MP, the 2-meter diagnostics are weighted as functions of their respective diagnostics over bare soil and over vegetation. The updated diagnostics for Noah and Noah-MP are now computed in the new file mpas_atmphys_sfc_diagnostics.F. (PR #1242) * Fix to provide consistency in the ringing behavior of recurring alarms after their reference time has been adjusted with a call to mpas_adjust_alarm_to_reference_time. Now, adjusting the reference time for an alarm will always leave that alarm in a state such that it is considered by the mpas_is_alarm_ringing routine to be ringing at the current time. With this fix, limited-area simulations can be restarted at times between LBC updates, provided the reference_time attribute for the 'lbc_in' stream is set to the simulation initial time in the streams.atmosphere file. (PR #1290). * Correction of an indexing error for rvcuten in code blocks specific to the Grell-Freitas scheme in the convection driver. Specifically, in the convection_from_MPAS and convection_to_MPAS routines, rvcuten used (k,k) as indexing in a loop, where (k,i) is needed. Since the Grell-Freitas scheme does not provide momentum tendencies, the changes in this merge have no impact on results. (PR #1283)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR aims to provide consistency in the ringing behavior of recurring alarms after their reference time has been adjusted with a call to
mpas_adjust_alarm_to_reference_time. Now, adjusting the reference time for an alarm will always leave that alarm in a state such that it is considered by thempas_is_alarm_ringingroutine to be ringing at the current time.For simplicity of discussion, consider only a clock that is running forward in time. The objective of the
mpas_adjust_alarm_to_reference_timeroutine is to set theprevRingTime(previous ring time) of a recurring alarm to (1) a time that differs from the specified reference time by an integer multiple of the alarm interval; and (2) the latest such time that is not after the current time on the clock.Prior to this PR, the logic in the
mpas_adjust_alarm_to_reference_timeroutine resulted in one of two outcomes:(1) If the difference between the reference time and the current time is divisible by the alarm interval, the
prevRingTimebecomes the current time, as illustrated below.(2) Otherwise, the
prevRingTimebecomes the latest time before the current time that lies on an integer multiple of the alarm interval away from the reference time, as illustrated below.To determine whether a recurring alarm is ringing, the alarm's interval is added to the alarm's
prevRingTime. If the result is on or before the current time, the alarm is ringing; otherwise, if the result is later than the current time, the alarm is not ringing. As a consequence, outcome (1) from thempas_adjust_alarm_to_reference_timeleads to an alarm that is ringing after the call to the routine, while outcome(2) leads to an alarm that is not yet ringing.In order to rectify the inconsistency in whether an alarm is ringing depending on where its reference time is set relative to the current time, the
prevRingTimefor an alarm is always set to be at least one full alarm interval before the current time. Whether the difference between the current time and the reference time is evenly divisible by the alarm's ring interval or not, a query of the alarm's status will always show that it is ringing.This PR makes changes to the logic for both a forward and a backward running clock in the
mpas_adjust_alarm_to_reference_time, although it appears that no code actually makes use of a backward running clock at present.This PR also adds unit tests for the
mpas_adjust_alarm_to_reference_timeroutine in thetestcore.