Skip to content

Issue #803: investigate missing depth coords#839

Open
mo-gill wants to merge 16 commits intomainfrom
803_investigate_missing_depth_coords
Open

Issue #803: investigate missing depth coords#839
mo-gill wants to merge 16 commits intomainfrom
803_investigate_missing_depth_coords

Conversation

@mo-gill
Copy link
Collaborator

@mo-gill mo-gill commented Mar 5, 2026

Closes #803

@mo-gill mo-gill self-assigned this Mar 5, 2026
@mo-gill mo-gill added the bug Something isn't working label Mar 5, 2026
mo-gill added 2 commits March 5, 2026 08:29
…ted separately for different frequencies. Breaks a lot of tests."

This reverts commit 28454c6.
@mo-gill
Copy link
Collaborator Author

mo-gill commented Mar 5, 2026

Simple fix for the main issue here.

However, once fixed i was getting some errors i found rather confusing. I have made a new issue for this, but provide more info below.

When running mip_convert after adding the depth coord fix i was getting these errors:

cmor_log:

! Error: The interval values used for the time axis differ from those defined for the frequency "day" used for by variable 'zostoga' (table ocean)

Mip_convert.log:

pywrapper.py", line 602, in variable
    return _cmor.variable(table_entry, units, ndims, axis_ids, str.encode(data_type),
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
_cmor.CMORError: Problem with 'cmor.variable'. Please check the logfile (if defined).
*** Finished conversions ***

This was due to two different frequencies being added in the same mip_convert cfg. And resulted in only one of the two output .nc files being generated.

In the mip_convert log this line was appearing twice:
variable arg: ["<class 'list'>"] "[0]"
And this at the end:

mip_convert.save.cmor.cmor_wrapper.CmorWrapper._debug_on_args DEBUG: close arg: ["<class 'NoneType'>"] "None"
mip_convert.save.cmor.cmor_wrapper.CmorWrapper.close DEBUG: close return "0"
mip_convert.request.convert INFO: *** Finished conversions ***

I think it may be use to cmor state not being cleared properly between the two jobs.

First i made an implementation to clear the state between each job, which worked. However as Matt mentioned this may have had performance impacts with large amounts of variables.

I tried to implement a way of using the frequency as a fingerprint as an alternative to keep the cmor states separate.
This worked and generated both the outputs, but i'm not convinced i implemented it the best way possible, as i was passing the key through quite a few classes in mip_convert. It also caused many tests to fail.


FAILED mip_convert/mip_convert/tests/test_convert/test_output/testAxisMakers.py::TestAxisExamples::testHybridHeight
FAILED mip_convert/mip_convert/tests/test_convert/test_output/testCmorDomain.py::TestGetAxisIds::testBareSoil
FAILED mip_convert/mip_convert/tests/test_convert/test_output/testCmorDomain.py::TestGetAxisIds::testErrorOnNonSingletonHeight
FAILED mip_convert/mip_convert/tests/test_convert/test_output/testCmorDomain.py::TestGetAxisIds::testErrorOnNonSingletonVegType
FAILED mip_convert/mip_convert/tests/test_convert/test_output/testCmorDomain.py::TestGetAxisIds::testHybridHeight
FAILED mip_convert/mip_convert/tests/test_convert/test_output/testCmorDomain.py::TestGetAxisIds::testSiteDomain
FAILED mip_convert/mip_convert/tests/test_convert/test_output/testCmorDomain.py::TestGetAxisIds::testVarSingletonHeightAndVegTye
FAILED mip_convert/mip_convert/tests/test_convert/test_output/testCmorDomain.py::TestGetAxisIds::testVegTypeDomain
FAILED mip_convert/mip_convert/tests/test_convert/test_output/testCmorOutputter.py::TestCmorOutput::testErrorOnIncompatibleVariableSize
FAILED mip_convert/mip_convert/tests/test_convert/test_output/testCmorOutputter.py::TestCmorOutput::testErrorOnLengthChange
FAILED mip_convert/mip_convert/tests/test_convert/test_output/testCmorOutputter.py::TestCmorOutput::testErrorOnLengthChangeAcrossFileBoundaries
FAILED mip_convert/mip_convert/tests/test_convert/test_output/testCmorOutputter.py::TestCmorOutput::testMultipleWritesToSingleFile
FAILED mip_convert/mip_convert/tests/test_convert/test_output/testCmorOutputter.py::TestCmorOutput::testMultipleWritesWithFileRefresh
FAILED mip_convert/mip_convert/tests/test_convert/test_output/testCmorOutputter.py::TestCmorOutput::testOfFxWrite
FAILED mip_convert/mip_convert/tests/test_convert/test_output/testCmorOutputter.py::TestCmorOutput::testWithComment
FAILED mip_convert/mip_convert/tests/test_convert/test_output/testCordexChunking.py::TestMipRequestVariable::testIsCordexMonthly
FAILED mip_convert/mip_convert/tests/test_convert/test_output/testCordexChunking.py::TestMipRequestVariable::testIsCordexSeasonal
FAILED mip_convert/mip_convert/tests/test_convert/test_output/testCordexChunking.py::TestMipRequestVariable::testIsCordexfx
FAILED mip_convert/mip_convert/tests/test_convert/test_output/testCordexChunking.py::TestMipRequestVariable::testIsMulti
========== 19 failed, 666 passed, 71 deselected, 20 warnings in 8.02s ==========

As in our workflows streams of two different frequencies aren't used in the same mip_convert run, this refactor is taking up too much time for little current benefit. Though it would be nice to implement this as a defensive refactor at some point if the problem of two frequencies in the same mip_convert.cfg ever might be encountered in future.

This is the commit that i reverted with the changes that fixed the issue, in case anyone is after a solution to that problem in future:
28454c6

@mo-gill
Copy link
Collaborator Author

mo-gill commented Mar 5, 2026

When looking at the mip convert logs, i noticed a bug where the frequency arg was incorrectly split and printed like this:

2026-03-05 08:39:34 mip_convert.save.cmor.cmor_wrapper.CmorWrapper._debug_on_args DEBUG: frequency arg: ["<class 'str'>"] "m"
2026-03-05 08:39:34 mip_convert.save.cmor.cmor_wrapper.CmorWrapper._debug_on_args DEBUG: frequency arg: ["<class 'str'>"] "o"
2026-03-05 08:39:34 mip_convert.save.cmor.cmor_wrapper.CmorWrapper._debug_on_args DEBUG: frequency arg: ["<class 'str'>"] "n"

After this little fix 401a465, this now prints correctly:

2026-03-05 08:55:06 mip_convert.save.cmor.cmor_wrapper.CmorWrapper._debug_on_args DEBUG: frequency arg: ["<class 'str'>"] "mon"

@mo-gill mo-gill marked this pull request as ready for review March 5, 2026 09:59
Copy link
Collaborator

@matthew-mizielinski matthew-mizielinski left a comment

Choose a reason for hiding this comment

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

I'm happy with the change here, but we should add a functional test for ocean/zostoga_tavg-u-hm-sea (monthly). I'm happy for that to be done on a separate ticket.

@mo-gill
Copy link
Collaborator Author

mo-gill commented Mar 6, 2026

I'm happy with the change here, but we should add a functional test for ocean/zostoga_tavg-u-hm-sea (monthly). I'm happy for that to be done on a separate ticket.

I decided to try and implement the functional test. Whilst working things out i ended up running the conversion via cdds_convert rather than mip_convert locally (which works). Turned out doing it via cdds_convert was failing. This commit to change a constant fixed it as cmor was complaining about a grid_label mismatch (g100 vs g100m). However, not sure if messing with that constant could affect other things.

The other commits since have been me trying to get the new monthly functional test working.

Fixed some issues but currently the test is throwing quite a few differences between the KGO and test output

AssertionError: The following differences were present: {'DIFFER : VALUES OF GLOBAL ATTRIBUTE : branch_time_in_parent : 3652 <> 0\nDIFFER : VALUES OF GLOBAL ATTRIBUTE : further_info_url : https://furtherinfo.es-doc.org/CMIP7.MOHC.UKCM2-0-LL.1pctCO2.none.r1i1p1f3 <> https://furtherinfo.es-doc.org/CMIP7.MOHC.UKCM2-0-LL.1pctCO2.none.r2i1p1f1\nDIFFER : VALUES OF GLOBAL ATTRIBUTE : grid : Native N96 grid; 192 x 144 longitude/latitude <> Native eORCA1 tripolar primarily 1 deg with meridional refinement down to 1/3 degree in the tropics; Global mean\nDIFFER : VALUES OF GLOBAL ATTRIBUTE : nominal_resolution : 250 km <> 100 km\n'}

I'll have a look at this after my annual leave next week. If anyone wants to check the request i used to generate the output i'll post a link on the chat.

@matthew-mizielinski
Copy link
Collaborator

AssertionError: The following differences were present: {
'DIFFER : VALUES OF GLOBAL ATTRIBUTE : branch_time_in_parent : 3652 <> 0
DIFFER : VALUES OF GLOBAL ATTRIBUTE : further_info_url : 
    https://furtherinfo.es-doc.org/CMIP7.MOHC.UKCM2-0-LL.1pctCO2.none.r1i1p1f3 <> 
    https://furtherinfo.es-doc.org/CMIP7.MOHC.UKCM2-0-LL.1pctCO2.none.r2i1p1f1
DIFFER : VALUES OF GLOBAL ATTRIBUTE : grid : 
    Native N96 grid; 192 x 144 longitude/latitude <> 
    Native eORCA1 tripolar primarily 1 deg with meridional refinement down to 1/3 degree in the tropics; Global mean
DIFFER : VALUES OF GLOBAL ATTRIBUTE : nominal_resolution : 250 km <> 100 km\n'}

Would be good to fix the grid attribute, but note that it may be disappearing from our files (no longer a standard global attributes) #ticket_needed. The others should be fixable with a replacement of the reference data.

mo-gill added 2 commits March 13, 2026 10:05
…stigate_missing_depth_coords

merge model fix
…803_investigate_missing_depth_coords"

This reverts commit a335c4b, reversing
changes made to 3c9dfaf.
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.

CMIP7: UKCM2: Investigate missing depth coordinates

3 participants