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

MLE: Use default answer date outside of block #570

Merged

Conversation

marshallward
Copy link
Member

Inside the MLE% ... %MLE parameter block, the default answer date was being read while inside the block. Since the namespaces inside blocks are not shared or inherited, this was creating a new parameter, MLE%DEFAULT_ANSWER_DATE, whose value differed from the global DEFAULT_ANSWER_DATE. This was causing unintuitive behavior of MLE%ML_RESTRAT_ANSWER_DATE, which ignored this global answer date.

This patch reads DEFAULT_ANSWER_DATE outside of the block, so that ML_RESTRAT_ANSWER_DATE now defaults to the global value.

@marshallward
Copy link
Member Author

Thanks to @jkrasting for discovering and reporting this behavior.

Copy link

codecov bot commented Feb 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 37.20%. Comparing base (e69d771) to head (e75da93).

Additional details and impacted files
@@            Coverage Diff            @@
##           dev/gfdl     #570   +/-   ##
=========================================
  Coverage     37.20%   37.20%           
=========================================
  Files           271      271           
  Lines         80475    80475           
  Branches      15008    15008           
=========================================
  Hits          29943    29943           
  Misses        44961    44961           
  Partials       5571     5571           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@marshallward marshallward force-pushed the mle_default_param_date_fix branch from 92cd113 to d8bef8b Compare February 27, 2024 18:25
@marshallward
Copy link
Member Author

I modified the PR to include do_not_log=.true. to the DEFAULT_ANSWER_DATE read, so that there won't be any changes to existing parameter files. But now I am wondering why it wasn't there in the first place.

Was it actually intended to be local to the block? If so, then I am not sure that is a good decision. But perhaps it was also just unintended.

@Hallberg-NOAA
Copy link
Member

DEFAULT_ANSWER_DATE was never intended to be local to the MLE% block. This bug slipped in because we changed the answers via a code change with the desired default settings, and hence ignored the (expected) regression failures, rather than following the proper procedure of adding a run-time parameter with a default value that preserves the older, undesirable answers the code, and then only changing the default after about a year has passed and there is a chance to change the answers via the input files. In this case, it would have required that we set the change-specific answer-date parameter without using DEFAULT_ANSWER_DATE for its default until went through the deliberately very slow default-change process. As you know, @marshallward, you and I have already received a much-deserved verbal berating over how we handled the pull request where this bug was introduced.

The do_not_log=.true. option would only be needed to avoid changing the parameters if this call were the first time that this parameter were being read, which should not be the case here, but it should be benign.

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.

I agree that this change corrects an actual bug, arising from the way that the module-specific parameter namespace is handled.

@Hallberg-NOAA Hallberg-NOAA added the bug Something isn't working label Feb 28, 2024
Inside the `MLE%` ... `%MLE` parameter block, the default answer date
was being read while inside the block.  Since the namespaces inside
blocks are not shared or inherited, this was creating a new parameter,
`MLE%DEFAULT_ANSWER_DATE`, whose value differed from the global
`DEFAULT_ANSWER_DATE`.  This was causing unintuitive behavior of
`MLE%ML_RESTRAT_ANSWER_DATE`, which ignored this global answer date.

This patch reads `DEFAULT_ANSWER_DATE` outside of the block, so that
`ML_RESTRAT_ANSWER_DATE` now defaults to the global value.
@Hallberg-NOAA Hallberg-NOAA force-pushed the mle_default_param_date_fix branch from d8bef8b to e75da93 Compare February 28, 2024 17:28
@Hallberg-NOAA
Copy link
Member

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

@Hallberg-NOAA Hallberg-NOAA merged commit f2e7abf into NOAA-GFDL:dev/gfdl Feb 28, 2024
14 checks passed
@marshallward marshallward deleted the mle_default_param_date_fix branch May 8, 2024 15:00
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.

3 participants