Skip to content

Conversation

@travissluka
Copy link
Contributor

@travissluka travissluka commented Oct 3, 2023

Description

changes:

  • the ctest test_soca_3dvar_godas now uses VertInterp instead of InsituTemperature and so should make use of VADER for the TL/AD
  • added a bunch of hacky looking code to do some variable renaming to make VADER happy. Don't look at this too much yet, it will be cleaned up a bit before this PR is merged, and will go away entirely when a follow up PR is done to clean up the variable names.

@Dooruk you should be able to use this to test your vader branch. In order for it to compile, I added a changeVarAD() empty stub, and commented out all the code in changeVarTL() and changeVarTraj() because they currently contain bugs.

Dependencies

build-group=https://github.com/JCSDA-internal/vader/pull/208

@Dooruk
Copy link
Collaborator

Dooruk commented Oct 4, 2023

Thanks @travissluka !

It's still not clear to me what atlas grid type would handle MOM6 tripolar grid (which structured type). For instance, ORCA has an add-on for atlas:

https://github.com/ecmwf/atlas-orca

Does tripolar MOM6 grid differ from ORCA dramatically that we don't need such add-on?

@danholdaway suggested using an atmospheric gaussian grid that VADER tests currently work for and just replace existing values with ocean variables. I can do that but i would like to have a permanent solution eventually.

@travissluka
Copy link
Contributor Author

@Dooruk I agree with Dan that it's probably easier to just use an existing gaussian grid. I don't think we need to go through the trouble of making sure the same tri-polar grid works with the vader ctests

@eap
Copy link
Contributor

eap commented Aug 31, 2025

Tests on this PR are disabled until you merge or run git cherry-pick 91300a4

@shlyaeva shlyaeva marked this pull request as ready for review December 30, 2025 17:39
@shlyaeva
Copy link
Collaborator

The code here is now updated to use vader recipe that @Dooruk introduced in https://github.com/JCSDA-internal/vader/pull/208 in linear variable changes.

There are still a few hacks here and there that I borrowed from @travissluka's original code, that we may be able to remove / simplify in the future. Another important point to note is that this approach (using VertInterp with vader variable change instead of InsituTemperature) doesn't work well with dual resolution. This is due to missing points generated by the interpolation to a different resolution that we don't handle properly (in any variable change to my knowledge). A disturbing problem for another day.

I think we should test all our current applications with this branch properly, before we proceed with merging, but please review when you have a minute. I'll start testing with our applications once I have some feedback.

@Dooruk
Copy link
Collaborator

Dooruk commented Dec 30, 2025

@shlyaeva thank you so much for moving this forward this is a lot of work in multiple repos. I like the idea of separating TL/AD from the NL part. I follow the changes in LinearVar.cc but don't have much to suggest there.

I tested the SOCA + VADER add_gswocean_tl branches using my sandbox setup against a build from 2 weeks ago. I used 0.25-deg resolution, Static 3DVAR, 10 iterations, XBT temperature and ARGO composite operator and the norm reduction results are identical. With VADER runtime was slightly longer. I suspect with more observation types this should favor towards VADER? I could test with a more realistic setup as well when I get a chance to give an approval.

@shlyaeva
Copy link
Collaborator

shlyaeva commented Jan 7, 2026

thank you so much for testing @Dooruk! can you share the logs from the two runs? I didn't expect the runtime to be much different, curious to see where it's happening. I'll test in our setup too.

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.

Use vader for temperature linear variable change

5 participants