Skip to content

Conversation

hkershaw-brown
Copy link
Member

@hkershaw-brown hkershaw-brown commented Oct 2, 2025

Description:

fix index passed to below sea floor
fix: dist calculation only after call to convert_vertical_state

Cannot assume convert_all_state_verticals_first is set to true assim_tools_mod
and even if you are doing horizonal distance only for localization, still need
to convert the state location to m to check against the depth of the ocean floor
in m.

Fixes issue

fixes #973

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

Documentation changes needed?

  • My change requires a change to the documentation.
    • I have updated the documentation accordingly.

Tests

Please describe any tests you ran to verify your changes.

Ran filter with 1 obs and 1 for the increment (⚠️ not this is not commited to the branch) to make spheres (with vertical localization on) or cylinders (for horizontal localization only). Using z* case. The plots are (output_to_filter.nc - input_to_filter.nc

diff --git a/assimilation_code/modules/assimilation/assim_tools_mod.f90 b/assimilation_code/modules/assimilation/assim_tools_mod.f90
index 0c595170b..9f57f91f3 100644
--- a/assimilation_code/modules/assimilation/assim_tools_mod.f90
+++ b/assimilation_code/modules/assimilation/assim_tools_mod.f90
@@ -2057,6 +2057,8 @@ endif

! Get the updated ensemble
if(.not. inflate_only) ens = ens + final_factor * increment
+ens = ens + final_factor * 1.0_r8
+

end subroutine obs_updates_ens
Screenshot 2025-10-02 at 10 01 39 AM Screenshot 2025-10-02 at 10 01 59 AM Screenshot 2025-10-02 at 10 02 47 AM

Checklist for merging

  • Updated changelog entry
  • Documentation updated
  • Update conf.py

Checklist for release

  • Merge into main
  • Create release from the main branch with appropriate tag
  • Delete feature-branch

Testing Datasets

  • Dataset needed for testing available upon request
  • Dataset download instructions included
  • No dataset needed

hkershaw-brown and others added 2 commits September 29, 2025 12:44
Cannot assume convert_all_state_verticals_first is set to true assim_tools_mod
and even if you are doing horizonal distance only for localization, still need
to convert the state location to m to check against the depth of the ocean floor
in m

see #973 (comment)

call loc_get_close_state(gc, base_loc, base_type, locs, loc_qtys, loc_indx, &
num_close, close_ind, dist, ens_handle)
num_close, close_ind)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm trying to understand the motivation for not computing the distances here. Is it faster to just get the indices and then compute the distances for the valid ones below?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes correct, the get_dist is the expensive bit of the calculation. This is the pattern a lot of our "bigger" models use.

Copy link
Contributor

@mgharamti mgharamti left a comment

Choose a reason for hiding this comment

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

Tested the changes by running filter with a single ob. It fixes the localization issue.
It's a clean change and Nicely done @hkershaw-brown!

@hkershaw-brown hkershaw-brown merged commit e5cfb8b into main Oct 10, 2025
4 checks passed
@hkershaw-brown hkershaw-brown deleted the fix-mom6-get-close-state branch October 10, 2025 13:50
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.

bug: MOM6 get_close_state incorrect index for ocean floor

2 participants