Skip to content

Conversation

@dfulu
Copy link
Member

@dfulu dfulu commented Jan 16, 2026

Pull Request

Description

There are a few edge case bugs in the validation function that this PR fixes.

e.g.

  • Some if statement like if hasattr(model, "nwp_encoders_dict"): which always evaluate to true since the model always has nwp_encoders_dict even if it is empty.
  • Looping through the NWP sources in the batch rather than looping through the NWP data that the model requires. The current version doesn't catch missing NWP sources. This new version does.

Plus some other light refactoring and clean ups

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

@dfulu dfulu added the bug Something isn't working label Jan 19, 2026
@dfulu dfulu changed the title Clean up validation functions Validation functions bugs and clean up Jan 19, 2026
@dfulu dfulu changed the title Validation functions bugs and clean up Validation functions bug and clean up Jan 19, 2026
Copy link
Member

@Sukh-P Sukh-P left a comment

Choose a reason for hiding this comment

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

Looks good, good spots and thanks for fixing this!

Had one small comment but not holding up merging, it's that the first edge case you described doesn't feel well tested still about nwp_encoders_dict always being present but sometimes being None. I am confident your changes will work for it but I guess the test coverage around that edge case still feels low

@dfulu
Copy link
Member Author

dfulu commented Jan 20, 2026

Looks good, good spots and thanks for fixing this!

Had one small comment but not holding up merging, it's that the first edge case you described doesn't feel well tested still about nwp_encoders_dict always being present but sometimes being None. I am confident your changes will work for it but I guess the test coverage around that edge case still feels low

That is true. We would have caught this before if we had that. I don't want that to hold up this PR though. So maybe it can be its own issue

@dfulu dfulu merged commit fa96d8d into main Jan 20, 2026
6 checks passed
@dfulu dfulu deleted the validation_clean branch January 20, 2026 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants