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

Add capability to create varobs and cx files for MTG-IRS #234

Merged
merged 9 commits into from
Feb 12, 2025

Conversation

stemiglio
Copy link
Contributor

Add MTG-IRS ObsGroup so that OpsMod_Control need to be moved under stubs to avoid removing the change when updating OPS. Also add necessary code to allow the creation of MTG-IRS varobs and the MTGIRS.nl namelists to for MTG-IRS varobs and cx files

@ctgh
Copy link
Collaborator

ctgh commented Feb 7, 2025

Thanks for adding this. Can you please add ctests for the varobs and CX writers?

@stemiglio
Copy link
Contributor Author

Thanks for adding this. Can you please add ctests for the varobs and CX writers?

Good point, thanks for reminding me. I have now added test_opsinputs_cxwriter_globalnamelist_mtgirs and test_opsinputs_varobswriter_globalnamelist_mtgirs ctests and I can confirm they succeed after compilation

Copy link
Collaborator

@ctgh ctgh left a comment

Choose a reason for hiding this comment

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

Thanks for adding the tests! I'm approving but I'd like to double check about the change in OPS code location. @adammaycock or @mikecooke77 can I check what you think please?

Copy link
Collaborator

@mikecooke77 mikecooke77 left a comment

Choose a reason for hiding this comment

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

All of the test files used with opsinputs should be created using the https://github.com/MetOffice/opsinputs/blob/develop/test/generate_unittest_netcdfs.py
script. This allows for reproducibility. This will result in changes to the ctest output which will need to be modified.

@stemiglio
Copy link
Contributor Author

Hi @mikecooke77, thanks for spotting the missing updated python file. I can confirm that I used the python script to generate the MTG-IRS test files, but I forgot to add the modified version to this PR. Now added. Note I tested that the varobs and cx files produced by the script are the same as those associated with this PR.

@mikecooke77
Copy link
Collaborator

Thanks for adding the tests! I'm approving but I'd like to double check about the change in OPS code location. @adammaycock or @mikecooke77 can I check what you think please?

I think this is the correct solution.

@ctgh
Copy link
Collaborator

ctgh commented Feb 10, 2025

@stemiglio When you're ready, please add the 'ready to merge' label and one of us can merge this.

@ctgh
Copy link
Collaborator

ctgh commented Feb 10, 2025

There is a CI failure but it seems to be something to do with python?

@stemiglio
Copy link
Contributor Author

Yes, I can see that. I'll do an empty commit to see if it goes away

@stemiglio
Copy link
Contributor Author

Nope, there is still this error:
CMake Error at ioda/src/engines/ioda/CMakeLists.txt:344 (target_link_libraries):

Target "ioda_engines" links to:

Python3::Python

but the target was not found. Possible reasons include:

* There is a typo in the target name.
* A find_package call is missing for an IMPORTED target.
* An ALIAS target is missing.

@matthewrmshin @mikecooke77 do you know what's the cause of it?

@stemiglio
Copy link
Contributor Author

@matthewrmshin I have now tried your suggestion and as you can see, with the older version of the JCSDA container the problem disappear. What do you suggest we do now?

@stemiglio
Copy link
Contributor Author

After liasing with @matthewrmshin I have now created #236. For now we can use the previous container image.

@matthewrmshin matthewrmshin merged commit 5c7d171 into develop Feb 12, 2025
6 checks passed
@matthewrmshin matthewrmshin deleted the feature/add_mtgirs branch February 12, 2025 14:50
@ctgh
Copy link
Collaborator

ctgh commented Feb 12, 2025

Thanks, merging now!

@ctgh
Copy link
Collaborator

ctgh commented Feb 12, 2025

Matt beat me to it! 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants