Skip to content

Conversation

wjbenfold
Copy link
Contributor

🚀 Pull Request

Description

Added a "reason" keyword argument to _coordinate_differences as currently it attaches the reason, and whilst it can diagnose some of the problem, it doesn't know if it's just been given metadata or also values to check, whereas the calling method does.

Fixes #4096

Question for reviewer:

  • Is this the best place to implement this fix? Should it be shallower or deeper instead?

Consult Iris pull request check list

@wjbenfold wjbenfold requested a review from rcomer October 22, 2021 12:40
@wjbenfold
Copy link
Contributor Author

@rcomer I've added you as reviewer as you raised the issue, but can get someone else on it instead if you'd prefer

@rcomer
Copy link
Member

rcomer commented Oct 30, 2021

Hi @wjbenfold, I’m happy to have a look at this, though I don’t think I’m qualified to answer this question:

Is this the best place to implement this fix? Should it be shallower or deeper instead?

Copy link
Member

@jamesp jamesp left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me. How many places is this function called in the codebase? Is it just the two you point out in #4096?
Do we need an additional test that picks up the default case?

@wjbenfold
Copy link
Contributor Author

How many places is this function called in the codebase?

It's called repeatedly in the method where it's called on scalar coordinates, but nowhere else I can see:

# Check dim coordinates.
if self.dim_metadata != other.dim_metadata:
differences = self._coordinate_differences(other, "dim_metadata")
msgs.append(
msg_template.format("Dimension coordinates", *differences)
)
# Check aux coordinates.
if self.aux_metadata != other.aux_metadata:
differences = self._coordinate_differences(other, "aux_metadata")
msgs.append(
msg_template.format("Auxiliary coordinates", *differences)
)
# Check cell measures.
if self.cm_metadata != other.cm_metadata:
differences = self._coordinate_differences(other, "cm_metadata")
msgs.append(msg_template.format("Cell measures", *differences))
# Check ancillary variables.
if self.av_metadata != other.av_metadata:
differences = self._coordinate_differences(other, "av_metadata")
msgs.append(
msg_template.format("Ancillary variables", *differences)
)
# Check scalar coordinates.
if self.scalar_coords != other.scalar_coords:
differences = self._coordinate_differences(
other, "scalar_coords", reason="values or metadata"
)
msgs.append(
msg_template.format("Scalar coordinates", *differences)
)

Do we need an additional test that picks up the default case?

This is already covered by the other tests in that file, e.g.

def test_aux_coords_metadata_difference_message(self):
cube_1 = self.cube
cube_2 = cube_1.copy()
cube_2.coord("foo").units = "m"
exc_regexp = "Auxiliary coordinates metadata differ: .* != .*"
with self.assertRaisesRegex(ConcatenateError, exc_regexp):
_ = concatenate([cube_1, cube_2], True)

@jamesp jamesp merged commit 40b6034 into SciTools:main Nov 10, 2021
@rcomer
Copy link
Member

rcomer commented Nov 10, 2021

Thanks for doing this @wjbenfold and @jamesp 🎉

@wjbenfold wjbenfold deleted the wjbenfold_concat_warn_scalar_coords branch November 10, 2021 14:58
tkknight added a commit to tkknight/iris that referenced this pull request Dec 7, 2021
* main: (23 commits)
  Suggest type hinting (SciTools#4390)
  area weight regrid test fixes (SciTools#4432)
  Update latest.rst (SciTools#4425)
  Added @wjbenfold to the core dev list (SciTools#4423)
  Removed addition of period from wrap_lons. (SciTools#4421)
  Add release docs sections describing the role of a Release Manager (SciTools#4413)
  Subset should always return None if no value matches are found (SciTools#4417)
  What's new for SciTools#4400 (SciTools#4422)
  `iris.analysis.AreaWeighted` regrid speedup (SciTools#4400)
  [pre-commit.ci] pre-commit autoupdate (SciTools#4419)
  Remove newline to satisfy setuptools (SciTools#4418)
  Updated environment lockfiles (SciTools#4416)
  NAME loader fixes (SciTools#4411)
  Updated whatsnew for PR 4402 (SciTools#4410)
  Support test data in benchmark workflows (SciTools#4402)
  What's new for pr 4387 (SciTools#4405)
  Make concat mismatch warning for scalar coords more accurate (SciTools#4387)
  Added line to latest release notes for updates to pp_save_rules.py (SciTools#4404)
  Update pp_save_rules.py (SciTools#4391)
  [pre-commit.ci] pre-commit autoupdate (SciTools#4403)
  ...
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.

Concatenate: wrong error message about scalar coords

3 participants