Skip to content

Numpy conversion consolidation function#396

Merged
felix-e-h-p merged 5 commits intomainfrom
numpy-conversion-consolidation
Feb 25, 2026
Merged

Numpy conversion consolidation function#396
felix-e-h-p merged 5 commits intomainfrom
numpy-conversion-consolidation

Conversation

@felix-e-h-p
Copy link
Copy Markdown
Contributor

@felix-e-h-p felix-e-h-p commented Feb 17, 2026

Consolidate numpy sample conversion into a single function - as referenced in #368 (think also related to #255)

Implemented single convert_to_numpy_sample(sample, t0) in convert.py:

  • Accepts dict of xarray objects that already exist in PVNet Dataset classes.
  • Modalities optional.
  • The {X}SampleKey classes all removed.

All related tests updated to use string keys instead of now old class attributes.

@felix-e-h-p felix-e-h-p requested a review from dfulu February 17, 2026 16:22
@felix-e-h-p felix-e-h-p marked this pull request as ready for review February 17, 2026 16:30
@dfulu dfulu mentioned this pull request Feb 18, 2026
7 tasks
Comment thread ocf_data_sampler/numpy_sample/convert.py Outdated
Comment thread ocf_data_sampler/numpy_sample/convert.py Outdated
Comment thread ocf_data_sampler/numpy_sample/convert.py
Copy link
Copy Markdown
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.

@felix-e-h-p this looks really good. Really nice to get rid of those SampleKey classes.

I've left a couple of comments. On the t0_idx I think we should strip it all out except for on the generation data. The original pipeline wasn't using those so I think we should just remove support for the NWP and sat t0_idxs. In the original we were just adding {"t0_idx": self.t0_idx} to each sample and I think we should just stick to that

Copy link
Copy Markdown
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.

Looks really good @felix-e-h-p. One last thing, can you strip out the merge_dicts() function since this change means we don't need it anymore

@felix-e-h-p felix-e-h-p merged commit efd90e3 into main Feb 25, 2026
6 checks passed
@felix-e-h-p felix-e-h-p deleted the numpy-conversion-consolidation branch February 25, 2026 10:05
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