Skip to content

Conversation

@amstokely
Copy link
Contributor

@amstokely amstokely commented Oct 1, 2025

This PR introduces user-defined active windows for recurring alarms in the MPAS timekeeping module. Previously, alarms could only be defined with an anchor time and a recurrence interval. Once the anchor was reached, the alarm would ring repeatedly until the end of the clock. There was no way to restrict alarms to a bounded time range.

Previous behavior

Alarms were defined only by anchor time and interval:

A ---------------- B ---------------------- E
^                 ^                        ^
Clock start    Alarm anchor              Clock stop

Once the anchor was reached, alarms rang every interval until the clock ended. There was no mechanism to disable alarms outside of a chosen window.

New behavior

With this PR, all alarms now have an active window. If the user does not explicitly provide a start or stop time, the window defaults to the clock start and stop times, so existing behavior is preserved. This allows the same logic to be applied consistently to both alarms with user-defined windows and alarms without them.

Timeline with user-defined window:

A                B          C            D                E
|----------------|----------|------------|----------------|
^                ^          ^            ^                ^
Clock start   Alarm anchor  Window     Window stop     Clock stop
                           start

Alarms do not ring before the window opens, ring normally inside it at the specified interval, and stop ringing after the window closes, even if the clock continues. Resets and direction changes interact correctly with these boundaries.

Internal structure

Several internal helpers were added to make the logic clearer and easier to maintain:

  • mpas_is_alarm_active checks whether the current clock time is inside an alarm’s active window.
  • mpas_prev_ring_in_window verifies whether the alarm’s previous ring time occurred strictly inside the window, using open-interval semantics (start, stop).
  • mpas_time_in_interval performs the low-level interval membership check, supporting both closed [start, end] and open (start, end) intervals.

These helpers are not tested directly; instead, their correctness is validated indirectly through behavioral tests of alarms. This ensures that the implementation can be refactored without breaking tests, as long as the observable alarm behavior remains consistent.

Tests

A new test fixture (alarm_fixture_t) and suite (test_window_alarm) validate alarm behavior across 17 scenarios. These cover cases before and after the anchor, ringing at the window boundaries, ringing inside the window, leaving the window, delayed anchor times, resets, and clock direction changes.

mpas_reset_clock_alarm is invoked in several cases to confirm that the previous ring time is updated correctly. It is called at both window boundaries and inside the active window to exercise different internal bound checks that could fail in one case but not the other.

The tests focus strictly on behavior: they verify only whether alarms ring when expected, not how that behavior is implemented. This design allows the internal code to evolve without destabilizing the suite. At the same time, the tests serve as executable documentation of the alarm ringing contract, which was previously implicit. They will make future development in MPAS timekeeping more efficient, reliable, and safe.

@amstokely amstokely marked this pull request as draft October 1, 2025 20:58
@amstokely amstokely force-pushed the framework/clock_alarm_active_window branch 2 times, most recently from 7d34528 to 1b12b02 Compare October 2, 2025 23:40
@amstokely amstokely force-pushed the framework/clock_alarm_active_window branch 2 times, most recently from cdc1d7c to 58b3e85 Compare October 3, 2025 17:59
@amstokely amstokely marked this pull request as ready for review October 3, 2025 18:04
@amstokely amstokely requested a review from mgduda October 3, 2025 18:22
@amstokely amstokely force-pushed the framework/clock_alarm_active_window branch from 83e24a2 to 65274f4 Compare October 23, 2025 23:02
@mgduda mgduda requested a review from jim-p-w October 23, 2025 23:32
@amstokely amstokely force-pushed the framework/clock_alarm_active_window branch from dd1f317 to b75ebbe Compare October 24, 2025 04:39
This commit adds active windows to recurring alarms. If users do not
provide a start or stop time, they default to the clock start and stop,
so existing behavior is unchanged. New helpers simplify the logic and
are validated indirectly through behavior-level tests, allowing future
refactoring without breaking the suite. A new test fixture and 16-case
suite verify alarm behavior across boundaries, resets, anchors, and
direction changes, documenting the alarm ringing contract and ensuring
safe future development of MPAS timekeeping.
@amstokely amstokely force-pushed the framework/clock_alarm_active_window branch from b75ebbe to 55fce8e Compare October 24, 2025 05:03
@jim-p-w
Copy link
Contributor

jim-p-w commented Oct 24, 2025

What is the use case?
Why would the window_stop time be before the clock_stop time? If the anchor occurs after the window_stop time the alarm will be missed.
Why would the window_start time be later than the clock_start time?

@mgduda
Copy link
Contributor

mgduda commented Oct 24, 2025

What is the use case? Why would the window_stop time be before the clock_stop time? If the anchor occurs after the window_stop time the alarm will be missed. Why would the window_start time be later than the clock_start time?

@jim-p-w The original motivation for enabling alarms to ring only within a specified window was to allow us to create output streams that are written only during part of a model simulation (i.e., within a window of time). The output times for a stream are controlled by alarms, and so starting with a change to the fundamental alarm functionality made sense. Generally when running MPAS, the starting and stopping times for the simulation clock -- within which alarms are instantiated -- are the starting and stopping times for the simulation.

@amstokely
Copy link
Contributor Author

What is the use case? Why would the window_stop time be before the clock_stop time? If the anchor occurs after the window_stop time the alarm will be missed. Why would the window_start time be later than the clock_start time?

@jim-p-w Here’s a snippet from a streams.atmosphere file that demonstrates how this feature is used:

<immutable_stream name="da_state"
                  type="output"
                  precision="single"
                  clobber_mode="truncate"
                  filename_template="mpasout.$Y-$M-$D_$h.$m.$s.nc"
                  packages="jedi_da"
                  io_type="pnetcdf,cdf5"
                  output_interval="0:40:00"
                  stop_time="0_03:20:00"/>

<stream name="da_state_low_freq"
        type="output"
        precision="single"
        clobber_mode="truncate"
        filename_template="mpasout_low_freq.$Y-$M-$D_$h.$m.$s.nc"
        packages="jedi_da"
        io_type="pnetcdf,cdf5"
        output_interval="1:00:00"
        start_time="0_04:20:00">
    <stream name="da_state"/>
</stream>

In this example, the da_state stream has an activeStartTime equal to the clock’s start time and an activeEndTime 3 hours and 20 minutes later, which is earlier than the clock’s end time. The da_state_low_freq stream begins 4 hours and 20 minutes after the start of the calculation and remains active until the clock’s end time.

The da_state stream writes output more frequently than da_state_low_freq. Although they are defined as separate streams, they write the same data and differ only in stream_name, filename_template, active window, and output frequency. This setup is useful in MPAS-JEDI, where users may want to vary the output frequency of the da_state stream during different phases of a model run.

@jim-p-w
Copy link
Contributor

jim-p-w commented Oct 24, 2025

What is the use case? Why would the window_stop time be before the clock_stop time? If the anchor occurs after the window_stop time the alarm will be missed. Why would the window_start time be later than the clock_start time?

In this example, the da_state stream has an activeStartTime equal to the clock’s start time and an activeEndTime 3 hours and 20 minutes later, which is earlier than the clock’s end time. The da_state_low_freq stream begins 4 hours and 20 minutes after the start of the calculation and remains active until the clock’s end time.

The da_state stream writes output more frequently than da_state_low_freq. Although they are defined as separate streams, they write the same data and differ only in stream_name, filename_template, active window, and output frequency. This setup is useful in MPAS-JEDI, where users may want to vary the output frequency of the da_state stream during different phases of a model run.

So start_time in the yaml is actually activeStartTime and stop_time is actually activeStopTime?

And what is the anchor time?

- Added a check in mpas_add_clock_alarm to return an error when
  activeStartTime occurs after activeStopTime.
- Updated test suite (case 18) to verify that alarms with reversed
  window times are rejected, while alarms with equal start and stop
  times are allowed.
@amstokely
Copy link
Contributor Author

What is the use case? Why would the window_stop time be before the clock_stop time? If the anchor occurs after the window_stop time the alarm will be missed. Why would the window_start time be later than the clock_start time?

In this example, the da_state stream has an activeStartTime equal to the clock’s start time and an activeEndTime 3 hours and 20 minutes later, which is earlier than the clock’s end time. The da_state_low_freq stream begins 4 hours and 20 minutes after the start of the calculation and remains active until the clock’s end time.
The da_state stream writes output more frequently than da_state_low_freq. Although they are defined as separate streams, they write the same data and differ only in stream_name, filename_template, active window, and output frequency. This setup is useful in MPAS-JEDI, where users may want to vary the output frequency of the da_state stream during different phases of a model run.

So start_time in the yaml is actually activeStartTime and stop_time is actually activeStopTime?

And what is the anchor time?

@jim-p-w The anchor time is an internally defined variable that serves as the reference point for the alarm’s time field. You’re also correct about the start_time and stop_time attributes. I find those names concise and intuitive for users. Internally, though, I avoided them in the timekeeping APIs because it’s clearer to think of an alarm as entering or exiting an active window rather than starting or stopping. The active window terminology makes this distinction explicit, since the start and stop values define when an alarm is active rather than when it begins or ends execution.

Replaced manual PASS/FAIL checks with assert_true and assert_false
subroutines for consistency and readability. Removed unused variables
and corrected argument intents to better reflect usage.
@amstokely amstokely requested a review from jim-p-w October 28, 2025 16:48
@amstokely amstokely requested a review from jim-p-w October 28, 2025 20:08
@amstokely amstokely requested a review from jim-p-w October 29, 2025 17:57
@amstokely amstokely force-pushed the framework/clock_alarm_active_window branch from 180ba41 to 94c91f8 Compare October 29, 2025 21:50
@amstokely amstokely requested a review from mgduda October 29, 2025 22:32
@amstokely amstokely force-pushed the framework/clock_alarm_active_window branch from 7dbc55e to 253c4f8 Compare October 30, 2025 17:18
type (MPAS_Alarm_type), pointer :: alarmPtr
integer :: threadNum

if (present(ierr)) ierr = ESMF_SUCCESS
Copy link
Contributor

Choose a reason for hiding this comment

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

Since existing code compares the value of ierr returned by this subroutine with 0 to determine success, I'd suggest we set ierr to 0 rather than to ESMF_SUCCESS.

integer, intent(out), optional :: ierr
type (MPAS_Time_type), intent(in), optional :: alarmStartTime
type (MPAS_Time_type), intent(in), optional :: alarmStopTime
integer, intent(out), optional :: ierr
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use just one space between intent(out), and optional.


implicit none

type(MPAS_Alarm_type), pointer :: alarm
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the alarm argument need to be a pointer here?


implicit none

type(MPAS_Alarm_type), pointer :: alarm
Copy link
Contributor

Choose a reason for hiding this comment

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

Does alarm need to be a pointer?

if (present(ierr)) ierr = 0

mpas_is_alarm_ringing = .false.

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's leave this blank line in place.

!-----------------------------------------------------------------------
! function mpas_prev_ring_in_window
!
!> \brief Check if the alarm’s previous ring was inside its window.
Copy link
Contributor

Choose a reason for hiding this comment

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

Here (and maybe elsewhere), can you check that we're using an ASCII single-quote rather than some other Unicode character?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants