NCAS-CMS / NCAS-CMS/cf-python

How best to handle the default (Earth) radius constant

Open
#941 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

code tidy enhancement question
Dominant language
Python
Stars
150
Forks
23
Avg merge
1d 11h
Merged PRs (30d)
2

Description

Throughout the codebase we often perform calculations using a value for the radius of a sphere. This can usually be assumed to be the Earth's because the relevant calculations concern the Earth's surface - but there may also be applications to extra-planetary atmospheres and oceans etc. so when referencing/naming the radius generally we should keep that wider scope in mind so as to not to possibly put off those wanting to do calculations for other planets.

During code review for #910 we decided to have a think about how best to handle the Earth radius default as a value that has a default to sensible precision (we use 6371229.0 meters generally), including as hard-coded at present as:

  • in mixin/fielddomain.py as a float constant (potnetially not ideal as a float as technically that can vary very slightly by floating point precision, as a separate concern);
  • a radius kwarg value default for cf.relative_vorticity.

See thread in https://github.com/NCAS-CMS/cf-python/pull/910#discussion_r2967106221 for original discussion points.

Suggestion

I suggest that:

  • we add earth_radius[_value] to the constants module as an integer value 6371229 and import there where necessary. The canonical units for it are already defined there:

    https://github.com/NCAS-CMS/cf-python/blob/8c760f82ed119cdda0ad041bc9deec49f3bc15d4/cf/constants.py#L18

    so it is nice to have it close to those as an additional benefit;

  • we change any radius reference to make sure it doesn't reference the 'Earth' in the name explicitly, but take our default Earth radius as default (the average user will be considering the Earth and surely in most cases not need to tweak the value from default precision);

  • for any functions that use the default Earth radius, consult that constant - notably where there's a corresponding keyword argument radius take 'earth' as a string default to mean 'use the default earth value', as we presently do for many methods such as Field.cell_area.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with cf/constants.py and mixin/fielddomain.py, then inspect the radius default in cf.relative_vorticity and the existing Field.cell_area convention. Trace other radius references that use the Earth value, and compare their naming and defaults. Done means the default value is centralized and the affected references consistently support the described Earth-radius behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
data
Issue type
Refactor
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.