Skip to content

Observation callsign changes for GPSRO#261

Merged
Matt Shin (matthewrmshin) merged 18 commits intodevelopfrom
feature/gnssro_des_o
Mar 11, 2026
Merged

Observation callsign changes for GPSRO#261
Matt Shin (matthewrmshin) merged 18 commits intodevelopfrom
feature/gnssro_des_o

Conversation

@liam-hall239
Copy link
Copy Markdown
Contributor

@liam-hall239 Liam Hall (liam-hall239) commented Nov 4, 2025

We found a bug in which opsinputs did not output the rising and setting flags for GPSRO into varobs.

We added a quality flag for the phase of occultation, rising (1) or setting (0), on the end of the observation callsign and split the last 7 characters of the callsign to include this flag at the end of the callsign.

An example is found in /home/users/liam.hall/desktop/opsinputs_pr/varobs.txt starting from line 1418.
The expected output is a 16 digit callsign with the last character to be either a 1 or a 0 - the phase flag.

@adammaycock
Copy link
Copy Markdown
Collaborator

Out of interest, how has this bug not caused a problem (difference from OPS). Does VAR do anything with the rising / setting flag?

Also, I assume this change will result in kgo differences in nightly sith / malak testing, so we'll need to run those tests and generate new kgo before merging this PR. If that's the case, we'll set the waiting for kgo update label on this PR.

@liam-hall239
Copy link
Copy Markdown
Contributor Author

Hi Adam, it hasn't caused any actual issues anywhere. Before, opsinputs cut off the very last digit of the callsign so the rising and setting information was lost at this point and is needed for current RO work.
The work I am doing now needs this flag and so I have made changes to my own version of VAR which splits the data into the respective occultation phase and then can be plotted.

For context, please see the graphs in ssa-desroziers-2025.

Copy link
Copy Markdown
Collaborator

@ctgh Chris Thomas (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 this fix. It looks like you need to add the new variable to the test netCDF file.

@DJDavies2
Copy link
Copy Markdown
Collaborator

One of the ctests is failing for me:

test_opsinputs_varobswriter_071_VarField_bendingangle

There are also some coding norms failures.

…hable else statement. Also tidied up to conform to coding norms.
@liam-hall239
Copy link
Copy Markdown
Contributor Author

I have just made a change to the initial commit to remove the else statement and include the variable in the constructor. I have also removed any white space from the file.

I have checked the varobs file to see if the change is implemented and it has, with the last digit of the callsign only showing 1s and 0s. It is located here: /home/users/liam.hall/desktop/opsinputs_pr/varobs_pr.txt for cycle 20250101T0000Z if you need a copy for the ctests.

@mikecooke77
Copy link
Copy Markdown
Collaborator

/home/users/liam.hall/desktop/opsinputs_pr/varobs_pr.txt

I believe for the ctests you will need to recreate the file using the python script (https://github.com/MetOffice/opsinputs/blob/develop/test/generate_unittest_netcdfs.py) as part of this PR.

@neill-b
Copy link
Copy Markdown
Contributor

So, the change tells me that 140 files have been modified. Surely this is not right?

@liam-hall239
Copy link
Copy Markdown
Contributor Author

Liam Hall (liam-hall239) commented Nov 17, 2025

I also thought that there were so many modified files.
When running the generate_unittest_netcdfs.py file, it gave: "PermissionError: [Errno 13] Permission denied: 'testinput/001_VarField_pstar.nc4' " so I deleted the existing file after checking my permissions and then it worked, creating this as well as modifying the rest.
The error never mentioned the other files.

@neill-b
Copy link
Copy Markdown
Contributor

It appears that this changeset is the rogue one: 89ff34b
I'd suggest that you revert that one, then only modify the files that need changes.

@liam-hall239
Copy link
Copy Markdown
Contributor Author

So to make sure I have this right, just push the generate_unittest_netcdfs.py and the file that gave an error or just the unittest python file and of course GnssroStationIDMetOffice.cc?

@mikecooke77
Copy link
Copy Markdown
Collaborator

Not sure what to make of the CI runner. Matt Shin (@matthewrmshin) is this because we are using an older version of the JCSDA container? Should we move this to just using the azure UKMO CI as it will only ever be run in this environment?

@matthewrmshin
Copy link
Copy Markdown
Collaborator

CI should now be using our own container running on our own self hosted runner.

@matthewrmshin
Copy link
Copy Markdown
Collaborator

Build is now failing with a compiler error:

/var/tmp/opsinputs/pr-261/opsinputs/src/opsinputs/GnssroStationIDMetOffice.cc:59:41: error: no matching function for call to 'missingValue(const int&)'
   59 |   const int missing = util::missingValue(missing);
      |                       ~~~~~~~~~~~~~~~~~~^~~~~~~~~
In file included from /tmp/build-and-test-j5MEtK/ioda/include/ioda/ObsDataVector.h:24,
                 from /var/tmp/opsinputs/pr-261/opsinputs/src/opsinputs/GnssroStationIDMetOffice.cc:24:

Copy link
Copy Markdown
Collaborator

@ctgh Chris Thomas (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, looks good!

@DJDavies2
Copy link
Copy Markdown
Collaborator

There are failures in the ctest test_opsinputs_varobswriter_071_VarField_bendingangle.

@ctgh
Copy link
Copy Markdown
Collaborator

Liam Hall (@liam-hall239) I think the new file doesn't contains MetaData/qualityFlags. I haven't checked it for sure but the failed ctest output indicates that may be the case.

 Please enter the commit message for your changes. Lines starting
@liam-hall239
Copy link
Copy Markdown
Contributor Author

Hi Matt Shin (@matthewrmshin), after speaking to Chris, it looks like the CI failure may be due to the recent
mobbs update. Local build and tests pass successfully from my workflow.

Please let me know if you're happy for the PR to be merged.

@matthewrmshin
Copy link
Copy Markdown
Collaborator

I've just hacked the CI image, so let's try building one more time.

@matthewrmshin
Copy link
Copy Markdown
Collaborator

I've hacked the CI build script to use Makefile not Ninja. This should work. Final try.

@matthewrmshin Matt Shin (matthewrmshin) merged commit b977130 into develop Mar 11, 2026
6 checks passed
@matthewrmshin Matt Shin (matthewrmshin) deleted the feature/gnssro_des_o branch March 11, 2026 16:07
@ctgh
Copy link
Copy Markdown
Collaborator

Matt Shin (@matthewrmshin) Thanks for getting it over the line.

@liam-hall239
Copy link
Copy Markdown
Contributor Author

Ahh success - Thank you all!

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.

8 participants