Skip to content

Confirm that nx*ny matches the number of elements in the model mesh#420

Draft
billsacks wants to merge 2 commits into
ESCOMP:mainfrom
billsacks:check_nx_ny
Draft

Confirm that nx*ny matches the number of elements in the model mesh#420
billsacks wants to merge 2 commits into
ESCOMP:mainfrom
billsacks:check_nx_ny

Conversation

@billsacks

Copy link
Copy Markdown
Member

Description of changes

Confirm that the model's specified nx*ny matches the number of elements in the model mesh.

At least in CESM, NX and NY come from XML variables like ATM_NX and ATM_NY. These are specified for pre-defined grids, but can be incorrect in some workflows with user-defined grids. Prior to this PR, this would sometimes be okay, but would sometimes cause hard-to-diagnose errors upon restart because fields on the CMEPS restart file would have incorrect sizes, leading to data corruption upon restart. This PR adds an explicit error check of this condition.

This currently bypasses the check for single-column runs because variables like ATM_NX and ATM_NY are currently set improperly for single-column runs due to #419 .

Code was written by Claude and thoroughly reviewed by myself.

Specific notes

Contributors other than yourself, if any:

CDEPS Issues Fixed (include github issue #):

Are there dependencies on other component PRs (if so list):

Are changes expected to change answers (bfb, different to roundoff, more substantial): bfb

Any User Interface Changes (namelist or namelist defaults changes): no

Testing performed (e.g. aux_cdeps, CESM prealpha, etc):

  • aux_cdeps: tests pass and bit-for-bit (but with a few BFAILs due to missing baselines)
  • aux_cime_baselines derecho tests: tests pass (except for two expected failures), didn't do baseline comparisons

Hashes used for testing: cesm3_0_alpha09d but with cmeps1.1.50

billsacks added 2 commits July 9, 2026 17:53
This is a workaround for ESCOMP#419 and
can probably be removed once that issue is resolved.
@billsacks

Copy link
Copy Markdown
Member Author

@mvertens - I'm especially interested in whether this (shr_strdata_init_from_config) feels like a reasonable place to put this check: I put it there because the model mesh size was available there... if it were instead put in dshr_mesh_init (where there were previously unused model_nxg and model_nyg arguments), I think we would have needed to duplicate the determination of the model mesh size there. Also note that I removed model_nxg and model_nyg from shr_strdata_type because they were unused... I considered setting them in shr_strdata_init_from_config, but then they would have been set for that path but not shr_strdata_init_from_inline, and that seemed confusing - and unnecessary since they aren't actually used.

@uturuncoglu / @DeniseWorthen - I'd be happy for one of you to run this in UFS testing to verify that this PR doesn't start triggering errors in your workflows: It's possible that you have nx and ny set incorrectly but that it's not actually causing any problems, and you'll start getting errors after this PR... so I'd like to wait to merge it until at least one of you gives me your okay.

All: @swensosc asked if it would make sense to add to the error message that the fix is to set the compname//"_NX" and compname//"_NY" XML variables correctly. However, I think those are specific to CESM/NorESM and not appropriate for UFS. What's the feeling on putting in place error messages that are helpful to users of some systems but irrelevant and possibly confusing for users of other systems? e.g., how would it be if I added a message like, "For CESM/NorESM, set these via './xmlchange ATM_NX' and './xmlchange ATM_NY'"?

@DeniseWorthen

Copy link
Copy Markdown
Collaborator

@billsacks Thanks for flagging this. We're behind on our CDEPS because of an issue created in PR #376 but it looks the code changes are simple enough I can just pick them up and test in UFS.

Comment on lines +266 to +267
if (.not. is_scol) then
if (model_nxg * model_nyg /= sdat%model_gsize) then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you replace is_col with model_gsize==1? Then there's no is_col flag that needs to be passed through

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I had considered that, but I prefer the current logic because it limits the error check bypass to the specific "single column" scenario, which follows special-purpose logic (which is currently broken, hence this bypass - see #419). I believe this is more specific than simply checking if model_gsize == 1. I wanted to keep this check active for other single-point cases if they were not subject to the broken single column logic - both so that errors could be reported in this case and to make the code more explicit that this bypass is just temporary until #419 is resolved.

Does that seem reasonable to you or do you have other reasons for preferring that this logic be changed?

An alternative is that we could wait for #419 to be resolved before merging this PR at all - at which point this single column bypass could be removed entirely... I'm just not sure when #419 can realistically be resolved.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ah, ok. whatever you think is best

@DeniseWorthen

Copy link
Copy Markdown
Collaborator

@billsacks I was able to run all our CDEPS related tests and everything passed, so I think it is good for UFS.

@billsacks

Copy link
Copy Markdown
Member Author

Thank you very much, @DeniseWorthen ! I will remove you and @uturuncoglu as requested reviewers given that test result.

@mvertens

Copy link
Copy Markdown
Collaborator

@billsacks - I agree that shr_strdata_init_from_config is the right place to put this check. However, I am wondering if we want to hold off on this PR until the relevant change is made so that is_col no longer appears as a conditional?

@billsacks

Copy link
Copy Markdown
Member Author

I am wondering if we want to hold off on this PR until the relevant change is made so that is_col no longer appears as a conditional?

Yes, now that we have a plan for how to fix the single-column logic, let's hold off on merging this until the single-column logic is fixed, so that I can remove this workaround.

I'll convert this to draft for now.

@billsacks
billsacks marked this pull request as draft July 15, 2026 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Check nx_global * ny_global against number of elements in mesh

4 participants