Skip to content

Updating CLI and Acceptance Tests for non-default scale factors. - #2374

Merged
mo-jbeaver merged 7 commits into
metoppv:masterfrom
mo-jbeaver:mobt_1151_validate_ecmwf_uv_index
Sep 28, 2026
Merged

mo-jbeaver merged 7 commits into
metoppv:masterfrom
mo-jbeaver:mobt_1151_validate_ecmwf_uv_index

Conversation

@mo-jbeaver

@mo-jbeaver mo-jbeaver commented May 6, 2026 •

Copy link
Copy Markdown
Contributor

Addresses #1151
Test Data: metoppv/improver_test_data#132

This PR implements changes to the uv_index CLI and Acceptance Test.
This provides the option to use a non-default scale-factor via the CLI.

Testing:

  • Ran tests and they passed OK
  • Added new tests for the new feature(s)

@mo-kbogue mo-kbogue left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I ran the acceptance test with no issues, though looking at the unit tests in improver_tests/uv_index/test_uv_index.py, I think that it may be worth adding some additional unit tests. There is already a unit test for non-default scale factor (in which 10 is used), but it might be worth taking other possibilities into account, such as including the case where a scale factor of 0 is received.

I would also like to have a better understanding and written record of where the 0.1 factor come from. From reading the previous tickets, it seems as is this possibly ported over from SPSS, but where did the 0.1 arise from in SPSS? If the justification is not clear, it may be worth revisiting if this is going to produce the best UV index for users. I do see that the jupyter notebook in the associated PR (#530) tests this 0.1 scaling and it does appear to give a reasonable answer, but is this enough of a scientific justification for its use? If we are confident in the 0.1 scaling factor, we should make sure that the reasons for this are clear and accessible.

Comment thread improver/cli/uv_index.py Outdated
@mo-jbeaver

Copy link
Copy Markdown
Contributor Author

In reference to the 0.1 scaling factor, the comment linked below adds some more context.
https://github.com/metoppv/mo-blue-team/issues/1143#issuecomment-4904360262

@mo-kbogue mo-kbogue left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks good to me, thanks for including that additional warning.

@mo-kbogue mo-kbogue left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Just one small typo, but otherwise this is great - thanks for this!

Comment thread improver/uv_index.py Outdated
scale_factor:
The uv scale factor. Default is 3.6 (m2 W-1). This factor has
been empirically derived and should not be
been empirically derived, or Met Office UM UV data, and should not be

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
been empirically derived, or Met Office UM UV data, and should not be
been empirically derived, for Met Office UM UV data, and should not be

mo-kbogue
mo-kbogue previously approved these changes Jul 23, 2026
@github-actions

Copy link
Copy Markdown

In order to maintain a backlog of relevant PRs, we automatically label them as stale after 60 days of inactivity.

If this PR is still important to you, then please comment on this PR and the stale label will be removed.

Otherwise this PR will be automatically closed in 30 days time.

@mo-jbeaver

Copy link
Copy Markdown
Contributor Author

These PRs (improver and improver_test_data changes) have been added to the board separately, so it can be second reviewed and merged independently. As the other UV Index PR (improver_aux) needs more work and investigation.

@maxwhitemet maxwhitemet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @mo-jbeaver. I have made a small suggestion to the warning output.

Comment thread improver/uv_index.py
Comment on lines +83 to +85
f"The scale_factor is not the default 3.6 (m2 W-1) but {scale_factor}. "
"This default factor has been empirically derived and should not be "
"changed except if there are scientific reasons to do so."

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Perhaps the following

Suggested change
f"The scale_factor is not the default 3.6 (m2 W-1) but {scale_factor}. "
"This default factor has been empirically derived and should not be "
"changed except if there are scientific reasons to do so."
f"The scale_factor is not the default 3.6 (m2 W-1) but {scale_factor}. "
"This default factor has been empirically derived and should not be "
"changed except if there are scientific reasons to do so."
"For more information, see this function's 'References' section."

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added

@mo-jbeaver mo-jbeaver assigned maxwhitemet and unassigned mo-jbeaver Sep 28, 2026
maxwhitemet
maxwhitemet previously approved these changes Sep 28, 2026

@maxwhitemet maxwhitemet left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Happy with the changes made. Approved 👍

@mo-jbeaver
mo-jbeaver merged commit 3c3f900 into metoppv:master Sep 28, 2026
7 checks passed
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.

3 participants