9 various tidy-ups and loud failing by dfulu · Pull Request #430 · openclimatefix/ocf-data-sampler

dfulu · 2026-07-31T16:02:55Z

Pull Request

Description

This PR wraps up a lot of small changes mainly aimed at the code failing early and raising helpful error messages

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 · 2026-07-31T16:09:22Z

+    """

-    @override
-    def __getattr__(self, item: str) -> NWP:


we don't use this and don't need it

dfulu · 2026-07-31T16:09:43Z

        return iter(self.root)

-    def keys(self) -> Iterator[str]:
+    def keys(self) -> KeysView[str]:


the old type hints were wrong

dfulu · 2026-07-31T16:12:22Z

            if time_periods is not None:
                mask = get_time_periods_mask(valid_t0_times, time_periods)
+                if not mask.any():
+                    raise ValueError(f"`time_periods` {time_periods} excluded all valid t0 times")


We raise this here instead of letting the dataset be empty and raise later further away from the source

dfulu · 2026-07-31T16:14:08Z

        )
+
+        if len(valid_t0_times) == 0:
+            raise ValueError(


This catches a separate error where there are some time periods available but none intersect with the t0 resolution. As in we could have a time period 00:10 -> 00:20 but if we have time resolution 30mins then we get no valid t0s

Was always silent / empty before right? Good one!

dfulu · 2026-07-31T16:14:53Z

        return valid_t0_times

    @staticmethod
    def find_valid_t0_and_location_ids(


I just did a bit of refactoring and cleaning up in this function

dfulu · 2026-07-31T16:17:01Z

-            valid_t0_per_location["location_id"] = location.id
-            valid_t0_and_location_ids.append(valid_t0_per_location)
+            if len(valid_t0_times) == 0:
+                logger.warning(f"No valid t0 times found for location {location.id}")


I think this is good to warn about since the model could silently train without ever seeing this location

This could have been an error but maybe that's too strict

dfulu · 2026-07-31T16:18:59Z

 ## Documentation

-**ocf-data-sampler** doesn't have external documentation _yet_; you can read a bit about how our torch datasets work in the README [here](ocf_data_sampler/torch_datasets/README.md).
+**ocf-data-sampler** doesn't have external documentation _yet_; you can read a bit about how our torch datasets work in the README [here](src/ocf_data_sampler/datasets/pvnet/README.md).


This was out of date

dfulu · 2026-07-31T16:20:54Z

    def __setstate__(self, state: dict) -> None:
        """Restore object from pickle, reloading from presaved file if possible."""
        self.__dict__.update(state)
-        if self._pickle_path and os.path.exists(self._pickle_path):


There was a chance here that this would fail weirdly if the pickled dataset was deleted or unavailable. So we handle this more explicitly now

dfulu · 2026-07-31T16:23:06Z

                max_staleness=max_staleness,
            )

-            if len(time_periods) == 0:


The find_contiguous_t0_periods[_nwp] functions raise their own errors if no periods are found, so we never reach these

Not reaching these was a bug, but perhaps we want to reach these. But that would require us to define what find_contiguous_t0_periods[_nwp] returns if there are no time periods. Is it just an empty dataframe?

dfulu · 2026-08-04T11:58:02Z

    for start_time, end_time in time_periods:

-        start_time = times[0] if start_time is None else np.datetime64(start_time)
-        end_time = times[-1] if end_time is None else np.datetime64(end_time)


The new version is slightly more robust and general since it doesn't rely on times being sorted and the [-1] indexer working on whatever input type goes in. This is so marginal that I'd wonder if its worth the code churn, so I'm happy to revert if we want.

One change here that we should have had is to be exclusive of the end point. A common pattern would be

train_period=[(None, "2025-01-01 00:00")] val_period=[("2025-01-01 00:00", None)]

This stops both train and val from containing the "2025-01-01 00:00" t0 time

dfulu · 2026-08-04T12:11:01Z

-        "date_cos": np.cos(date_in_radians),
-        "time_sin": np.sin(time_in_radians),
-        "time_cos": np.cos(time_in_radians),
+        "date_sin": np.sin(date_in_radians).astype(np.float32),


These were returning float64 which is not what the function signature advertises. We also cast to float32 for the TensorBatch passed to the model

dfulu · 2026-08-04T12:13:04Z

            ("2023-01-01 12:00", "2023-01-01 13:00"),
        ],
    )
-    expected_mask = np.array([False, True, True, True, False, True, True, True])


The end point is now exclusive

Clarified randomness behavior in README regarding dropout and NumPy state.

Clarified the docstring for the presave_pickle method.

felix-e-h-p · 2026-08-08T12:38:57Z

    return da


+def _assert_steps_uniformly_spaced(steps: NDArray[np.timedelta64], provider: str) -> None:


This will cause a hard fail with uneven steps say for MO right?

felix-e-h-p · 2026-08-08T12:43:41Z

-                mask = get_time_periods_mask(valid_t0_and_location_ids["t0"], time_periods)
-                valid_t0_and_location_ids = valid_t0_and_location_ids[mask]
+                mask = get_time_periods_mask(valid_t0_and_location_ids["t0"].values, time_periods)
+                if not mask.any():


felix-e-h-p · 2026-08-08T12:50:53Z


+    # Each init-time is unusable before init_start_timedelta and after init_end_timedelta. If they
+    # are the wrong way round then no t0 can ever use it, and every period below will be empty
+    if init_end_timedelta < init_start_timedelta:


Solid one, clear flagging

felix-e-h-p · 2026-08-08T12:51:49Z

    Returns:
        The selected DataArray-like slice.
    """
+    if len(locations) == 0:


For any window inversion errors right?

felix-e-h-p

Thanks for this! Looking great. Think the one thing from me would be if the uniformly spaced steps assertion could reject archives, but all good asides that.

dfulu changed the base branch from main to 8_split_generation July 31, 2026 16:06

dfulu commented Jul 31, 2026

View reviewed changes

Comment thread src/ocf_data_sampler/datasets/pvnet/dataset.py Outdated

dfulu commented Jul 31, 2026

View reviewed changes

dfulu commented Aug 4, 2026

View reviewed changes

dfulu force-pushed the 9_various branch from bad6239 to 0c9203d Compare August 4, 2026 12:17

dfulu marked this pull request as ready for review August 4, 2026 12:17

dfulu changed the title ~~9 various -TBC~~ 9 various tidy-ups and loud failing Aug 4, 2026

felix-e-h-p self-requested a review August 4, 2026 15:11

dfulu mentioned this pull request Aug 6, 2026

dev - next major release #400

Draft

Base automatically changed from 8_split_generation to dev_feb2026_speedups August 7, 2026 13:16

dfulu and others added 10 commits August 7, 2026 13:25

Fix t0 filtering for None

4a56094

Make datetime-encoding match t0 embedding calculations

2ace3c0

Make fill value use clearer

7ab3777

Fix multi-nwp config

d781bc4

Explicitly limit to NWPs with uniform steps

ea0ab33

Make pickle fail louder if path is missing

374765a

Add note on randomness

8674a5e

Catch and raise errors early

bdd1199

Apply suggestion from @dfulu

8ebcfe9

Update randomness section in README.md

6138c4c

Clarified randomness behavior in README regarding dropout and NumPy state.

dfulu and others added 3 commits August 7, 2026 13:25

Update docstring for presave_pickle method

38d82a6

Clarified the docstring for the presave_pickle method.

lint

dbfc10f

lint

014ec7b

dfulu force-pushed the 9_various branch from f00594c to 014ec7b Compare August 7, 2026 13:25

felix-e-h-p reviewed Aug 8, 2026

View reviewed changes

felix-e-h-p requested changes Aug 8, 2026

View reviewed changes

		return da


		def _assert_steps_uniformly_spaced(steps: NDArray[np.timedelta64], provider: str) -> None:

Uh oh!

Uh oh!

Conversation

dfulu commented Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request

Description

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfulu Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dfulu Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felix-e-h-p left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dfulu commented Jul 31, 2026 •

edited

Loading

dfulu Jul 31, 2026 •

edited

Loading

dfulu Aug 4, 2026 •

edited

Loading