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

Bug fix for subroutine write_energy when DT<2 #749

Merged

Conversation

herrwang0
Copy link

This PR fixes a bug with subroutine write_energy when using a DT<2.

Otherwise, the energy outputs could be written at wrong time steps with extremely small time stepping size.

The reason was that time type divide is essentially a floor. So in the line in question, dt_force/2 = 0 if dt_force<2. And energy is always written at the following time step, rather than when day==CS%write_energy_time.

The patch is a bit ugly.

  1. I believe both dt_force/2 and <= (rather than <) are necessary for round-off considerations.
  2. There is no real divide for time_type and a real number. (operator(//) is between time_type variables and operator(*) is for a time_type and an integer)

@Hallberg-NOAA
Copy link
Member

The time type allows for the use of sub-second time increments (ticks), which can be set during initialization via the FMS routine set_ticks_per_second() to allow for very short (sub-second) time steps, although it should be noted that unless set_ticks_per_second() has been called, there is one tick per second. Would the use of this capability be a better solution to this problem?

Also, please explain your comment that just changing elseif (day + (dt_force/2) <= CS%write_energy_time) then to elseif (day + (dt_force/2) < CS%write_energy_time) then would not suffice to fix this bug and do the right thing in (almost?) every case.

@herrwang0
Copy link
Author

The time type allows for the use of sub-second time increments (ticks), which can be set during initialization via the FMS routine set_ticks_per_second() to allow for very short (sub-second) time steps, although it should be noted that unless set_ticks_per_second() has been called, there is one tick per second. Would the use of this capability be a better solution to this problem?

Yes, we will definitely have more flexibility if could use set_ticks_per_second() to set ticks_per_second>1. Unfortunately, many FMS related features do not support a non-default ticks_per_second. In particular, subroutine get_time() need to have an explicit ticks output if the input time_type variable has non-zero number of ticks (i.e.ticks_per_second>1). This is often not the case when get_time() is called in, for example, diagnostics.

I think MOM6 supports using time steps with a fractional second, but the diagnostics could be off the mark (in time axis literally) due to bugs in FMS. This is probably a case beyond the scope of this PR and MOM6 and needs further changes in FMS.

Also, please explain your comment that just changing elseif (day + (dt_force/2) <= CS%write_energy_time) then to elseif (day + (dt_force/2) < CS%write_energy_time) then would not suffice to fix this bug and do the right thing in (almost?) every case.

Upon further examination and considering that all time_type attributes are integers, the solution with a simple < should work. The PR is now modified accordingly.

Copy link
Member

@Hallberg-NOAA Hallberg-NOAA left a comment

Choose a reason for hiding this comment

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

This is now a parsimonious code solution to the problem identified in the discussion of this PR.

@Hallberg-NOAA Hallberg-NOAA added the bug Something isn't working label Nov 29, 2024
Fix a bug with subroutine write_energy when using a DT<2. Otherwise,
the energy outputs are written at wrong time steps.

The reason was that time type divide is essentially a floor.
So DT/2 = 0 if DT<2.
@Hallberg-NOAA
Copy link
Member

This has passed pipeline testing at https://gitlab.gfdl.noaa.gov/ogrp/mom6ci/MOM6/-/pipelines/25608.

@Hallberg-NOAA Hallberg-NOAA merged commit 51b4fb6 into NOAA-GFDL:dev/gfdl Nov 30, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants