Skip to content

Add Var.are_compatible for FlatVar and Metadata#409

Open
ph-kev wants to merge 2 commits into
mainfrom
kp/flatten-compatible
Open

Add Var.are_compatible for FlatVar and Metadata#409
ph-kev wants to merge 2 commits into
mainfrom
kp/flatten-compatible

Conversation

@ph-kev

@ph-kev ph-kev commented Jun 29, 2026

Copy link
Copy Markdown
Member

This functionality is needed in ClimaCalibrate for comparing two different FlatVar or Metadata and determining whether they are compatible on physical space and the values dropped are on the same coordinates. The function arecompatible for FlatVar and Metadata is needed to avoid accessing drop_mask from the Metadata which is internal to ClimaAnalysis.

@ph-kev
ph-kev force-pushed the kp/flatten-compatible branch from 8217db3 to 699bfa3 Compare June 29, 2026 17:52
@ph-kev
ph-kev requested a review from nefrathenrici June 29, 2026 17:52
@codecov

codecov Bot commented Jun 29, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.50%. Comparing base (db0b943) to head (471b642).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #409   +/-   ##
=======================================
  Coverage   99.49%   99.50%           
=======================================
  Files          19       19           
  Lines        2181     2201   +20     
=======================================
+ Hits         2170     2190   +20     
  Misses         11       11           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nefrathenrici nefrathenrici left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks good. I'm a bit confused about how ignore_dims is meant to be used, since there are three checks and it only blocks one. For example, if vars differ in the flatten order or the drop mask, arecompatible will return false even though the specific dimension has been "ignored". The docstring is precisely worded, but it would be good to add a single line that differing flatten orders or drop masks will make the vars incompatible.

@ph-kev

ph-kev commented Jun 29, 2026

Copy link
Copy Markdown
Member Author

This looks good. I'm a bit confused about how ignore_dims is meant to be used, since there are three checks and it only blocks one. For example, if vars differ in the flatten order or the drop mask, arecompatible will return false even though the specific dimension has been "ignored". The docstring is precisely worded, but it would be good to add a single line that differing flatten orders or drop masks will make the vars incompatible.

I am confused on what three checks you are referring to. One way of thinking about what it means for two FlatVars to be arecompatible is that it makes sense to do operations (addition) with them. I don't think it makes sense to ignore the dimensions in the flatten order since it wouldn't really make sense to do operations between them. Similarly, if values are on different coordinates, it also doesn't make sense to do operations with them. So, the only purpose of ignore_dims is to give people an out when ignoring coordinate values of the dimensions (and since the check is in for loop, also ignoring the dimension units).

@ph-kev
ph-kev force-pushed the kp/flatten-compatible branch from 699bfa3 to 061b023 Compare July 1, 2026 23:26
@nefrathenrici

nefrathenrici commented Jul 2, 2026

Copy link
Copy Markdown
Member

I am confused on what three checks you are referring to. One way of thinking about what it means for two FlatVars to be arecompatible is that it makes sense to do operations (addition) with them. I don't think it makes sense to ignore the dimensions in the flatten order since it wouldn't really make sense to do operations between them. Similarly, if values are on different coordinates, it also doesn't make sense to do operations with them. So, the only purpose of ignore_dims is to give people an out when ignoring coordinate values of the dimensions (and since the check is in for loop, also ignoring the dimension units).

Ok, thanks for the explanation, the behavior makes sense. The three checks I meant are: flatten order, per-dimension coordinate values + units, and the drop mask. A differing flatten order or drop mask would make the vars incompatible so we shouldn't ignore them.

@ph-kev
ph-kev force-pushed the kp/flatten-compatible branch from 061b023 to 471b642 Compare July 10, 2026 22:24
@ph-kev

ph-kev commented Jul 13, 2026

Copy link
Copy Markdown
Member Author

Note that this PR haven't been merged since CliMA/ClimaCalibrate.jl#334 isn't merged in. Once that PR is ready to be merged, this PR can be merged in.

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.

2 participants