Skip to content

Conversation

@aouatefm
Copy link

Pull Request

Description

This PR adds backward compatibility support for UKV variable name changes (#364).

Problem: New UKV data uses different dimension names (x_osgb, y_osgb) while legacy data uses (x, y). The current code always renames xx_osgb and yy_osgb, which breaks with the new dataset.

Solution: Modified the open_ukv function to conditionally rename spatial coordinates only when legacy dimension names (x, y) are present. This maintains compatibility with both:

  • Legacy data (renames xx_osgb, yy_osgb)
  • New data (no renaming needed since dimensions are already x_osgb, y_osgb)

How Has This Been Tested?

  • Tested with both legacy and new UKV dataset formats
  • Verified spatial coordinates are properly handled in both cases
  • Confirmed the function doesn't break existing workflows

If your changes affect data processing, have you plotted any changes? i.e. have you done a quick sanity check?

  • Yes - verified data structure and coordinate naming in both scenarios

Checklist:

  • My code follows OCF's coding style guidelines
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked my code and corrected any misspellings

@AvinashYerra
Copy link

Hi @aouatefm, I am not a maintainer but wanted to understand how did you come up with ds.dims(). Is this something from tensorflow or will get to know if we setup the project?

"y": "y_osgb",
},
)
# Build rename dictionary conditionally based on existing dimensions
Copy link
Member

Choose a reason for hiding this comment

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

@aouatefm can you remove this comment. I don't think it improves the clarity of the code

Copy link
Member

@dfulu dfulu left a comment

Choose a reason for hiding this comment

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

@aouatefm thanks for this contribution. I've left one comment, but after that this looks good to merge

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