Skip to content

Conversation

@liam-hall239
Copy link

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
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 liam-hall239 requested a review from orlewis November 4, 2025 17:37
@liam-hall239
Copy link
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
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 this fix. It looks like you need to add the new variable to the test netCDF file.

@DJDavies2
Copy link
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
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
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
Contributor

neill-b commented Nov 17, 2025

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

@liam-hall239
Copy link
Author

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
Contributor

neill-b commented Nov 17, 2025

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
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?

@neill-b
Copy link
Contributor

neill-b commented Nov 17, 2025

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?

I would assume that the only data file to change should be test/testinput/071_VarField_bendingangle.nc4. I don't understand why the python code to generate the unit tests has altered the other data files, it shouldn't need to. That might be a question for the code owner of the python. But anyway, I'd try reverting the changes on all the other files.

@ctgh
Copy link
Collaborator

ctgh commented Nov 17, 2025

This has happened before - it seems to occur whenever the underlying software stack is updated, which causes small differences in the netCDF files. Previously we have just updated all of the files even if they are unrelated to the PR.

@mikecooke77
Copy link
Collaborator

Not sure what to make of the CI runner. @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
Collaborator

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

@matthewrmshin
Copy link
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
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, looks good!

@DJDavies2
Copy link
Collaborator

There are failures in the ctest test_opsinputs_varobswriter_071_VarField_bendingangle.

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.

9 participants