Use crs.globe in _crs_distance_differentials() instead of creating a new Globe

[dennissergeev]

🚀 Pull Request

Description

Addresses the first point in #4582 by making iris.analysis.cartography._crs_distance_differentials() use the .globe attribute of the input crs parameter instead of creating a new ccrs.Globe() object.

All tests pass on my computer.

---

Consult Iris pull request check list

[wjbenfold]

Thanks for this PR @dennissergeev!

Just one addition it would be good to get in:

Testing

It's great that the tests all pass, though given they passed beforehand that's probably indicative of our test coverage rather than anything having improved. Let's change that by adding a test - my vote below, though if somewhere else seems more suitable then go for it.

I'd add the following or similar to lib/iris/tests/unit/analysis/cartography/test_rotate_winds.py

class TestNonEarthPlanet:
    def test_different_planet(self):
        u, v = uv_cubes()
        u.coord("grid_latitude").coord_system = iris.coord_systems.GeogCS(1)
        u.coord("grid_longitude").coord_system = iris.coord_systems.GeogCS(1)
        v.coord("grid_latitude").coord_system = iris.coord_systems.GeogCS(1)
        v.coord("grid_longitude").coord_system = iris.coord_systems.GeogCS(1)
        rotate_winds(u, v, iris.coord_systems.GeogCS(1))

I considered testing closer to the changed function but it doesn't have coverage currently as it's a private function so I think checking the rotate_winds function is a good way to go.

Before we merge this we'll also want to retarget the branch to a patch release to Iris 3.2 (probably 3.2.1) and add a what's new entry in the appropriate place to let people know what's changed, but we can worry about that another day.

[dennissergeev]

Thanks for reviewing my PR @wjbenfold!

At your suggestion, I've added a test case, I hope it's good enough.

[wjbenfold]

Yep, better than my suggestion and it fails without your change but passes with it.

I'll check about putting it in a patch release rather than straight onto Iris main so we can get the benefit out there earlier, so don't merge it yet.

[dennissergeev]

Many thanks for your help @wjbenfold!

[lbdreyer]

@dennissergeev could you retarget this to target the v3.2.x branch instead of main?

And could you also add a whats new entry to the "Bugs fixed" section in the "3.2.1" Patches drop down box. You may need to rebase your branch onto the v3.2.x to do this.

Once those two are done it looks likes this would be ready to be merged

[dennissergeev]

@lbdreyer sorry, what's the best way of rebasing my PR to the v3.2.x branch?

[lbdreyer]

The commands I use are:

git fetch upstream
git rebase --onto upstream/v3.2.x

Where upstream refers to the Scitools/iris repo

$ git remote -v
origin	[email protected]:lbdreyer/iris.git (fetch)
origin	[email protected]:lbdreyer/iris.git (push)
upstream	[email protected]:SciTools/iris.git (fetch)
upstream	READONLY (push)

Assuming that works, you should then be able to edit the whats new and commit that change. Then when it comes to pushing your changes, the following command may not work:

git push origin <name_of_branch>

this is because your local branch is different (due to it now being based off v3.2.x rather than main) compared to the remote branch you are trying to update. If this is the case, you will get an error saying it failed with a message, something like "To prevent you from losing history, non-fast-forward updates were rejected ". In this case you can add the --force option:

git push origin <name_of_branch> --force

And that will force it to push you branch

Hope that helps!

[dennissergeev]

Thanks for your help @lbdreyer, but I think something went wrong... I only now noticed I was using the master branch instead of main because I had a fork of iris before you renamed the main branch. When I force-pushed the master branch, the commits changed and my changes seem to be lost...

[dennissergeev]

I'm going to do it again starting from the v.3.2.x branch.

[lbdreyer]

I'm going to do it again starting from the v.3.2.x branch.

Sounds like a good plan :) In case you can't remember what you changed the two commits were:
e58a315 and bc8e1c1