Add smaller fixes to make running from files more robust by JoachimKoenigslieb · Pull Request #15 · dmidk/sunflow

JoachimKoenigslieb · 2026-06-15T13:12:50Z

I wanted to use sunflow to do nowcasting over the US with data from GOES.

I ultimately get the data from this public AWS source here https://registry.opendata.aws/noaa-goes/.

Unfortunately my files are stored converted to grib2, so I'm getting into some quite off-label usage here. Maybe this is not something you guys are interested in supporting up stream here? The specific changes needed to support it are quite general and feels like reasonable changes tough.

Turns out that xarray is happy enough to read my files out of the box if I install cfgrib as a peer-dependency of sunflow. One big issue remains: I'm not storing the clearsky in these grib2 files! Instead I made a small config switch that allows for fetching the clearksy via the already-installed pvlib.

A list of fixes implemented are:

Make minutes available for formatting file names
use .expand_dims instead of .assign_coords to make sure we have both time dimension AND time coordinates when loading
Added a helper make_pvlib_clearsky_dataset which makes clearsky by calling pvlib. Also added a new config variable clearsky_source which defaults to file and switches behavior when set to pvlib.
check_solar_elevation now does not take Copenhagen as a default as that can easily lead to silent buggy behavior.

With these changes I'm able to do nowcasts over CONUS:

…nt skip out on forecasts on the borders of the domain near sunset/sunrise

KristianHMoller · 2026-06-26T09:11:21Z

Thanks a lot @JoachimKoenigslieb, this is awesome!
For my first quick test of the pvlib clear sky values, it worked perfectly.
One question: Is there a particular reason for your choice of the pvlib.clearsky.simplified_solis values for the clear-sky? I just noticed that the library had multiple options: https://pvlib-python.readthedocs.io/en/stable/reference/clearsky.html . But I can see that some options require additional input.

KristianHMoller · 2026-06-26T09:20:14Z

In general, I think that support for reading in data from GRIB files would also be nice. Is it correctly understood, that this would require only cfgrib included as a dependency in the pyproject.toml file?

KristianHMoller · 2026-06-26T09:23:16Z

        {month}: Two-digit month (e.g. 03)
        {day}: Two-digit day (e.g. 09)
        {hour}: Two-digit hour (e.g. 12)
+        {minute}: Two-digit minute (e.g. 30)


Perhaps, we could add second as well here and below, to have full flexibility?

Sounds good. Will add this! (probably next week tough :)

Regarding if all that is needed is cfgrib to read grib files: Yes, but this dependency is also quite hefty. I think it brings in eccodes itself as far as I understand.

For pvlib, this was the only way (from not much looking admittedly) of getting a field of clearsky values. Other methods in pvlib wanted some position tuples which would probably be quite slow I think.

Good point with cfgrib - no need to make the suite unnecessarily big!
And fair with the clearsky methods - I was mainly curious

KristianHMoller · 2026-06-29T11:35:45Z

+            for lon in (lon_min, lon_max)
+        )
+        logger.info(f"Maximum corner solar elevation: {solar_elevation:.2f} degrees")
        if solar_elevation < 1:


I wonder, if the value of 1 degree should be increased a bit now that we verify the corner with the maximum elevation. For instance, the MSG-CPP dataset is only available for solar elevation greater than 6 degrees. Is 1 degree useful for your applications @JoachimKoenigslieb ?

Below 6 degrees for maximum corner solar elevation, running with MSG-CPP input fails with:
Nowcast failed for 2026-06-29T02:00:00Z: precip contains only non-finite values

So if lower values than that are needed, it would need to be modifiable (which may be the most elegant solution) by either an argument or a environment variable.

KristianHMoller · 2026-06-29T12:00:50Z


 from .geospatial import get_coordinates

+PVLIB_CLEARSKY_VARIABLE_NAME = "pvlib_clearsky_ghi"


I suggest moving this to config.py and importing from that, both here and in main.py. I just tested that and it seems to work without an issue.

KristianHMoller · 2026-06-29T12:03:13Z

The command: pre-commit run --all should fix most linting issues and highlight the ones it cannot automatically fix.

KristianHMoller · 2026-06-29T12:31:54Z

+
+    fields = []
+    for time in times:
+        repeated_times = pd.DatetimeIndex([time] * len(flat_latitudes))


Suggested change

repeated_times = pd.DatetimeIndex([time] * len(flat_latitudes))

repeated_times = [time] * len(flat_latitudes)

This seems to yield identical results and does not require pandas import into this file. But as pandas is already installed, either option is fine

KristianHMoller · 2026-06-29T12:49:48Z

+    times: list[datetime],
+    latitudes: np.ndarray,
+    longitudes: np.ndarray,
+) -> xr.Dataset:


Suggested change

) -> xr.Dataset:

) -> xr.Dataset:

"""Compute a pvlib simplified-Solis clear-sky GHI dataset on a lat/lon grid.

For each timestep in `times`, solar position is computed at every grid

point by repeating the timestamp across all (lat, lon) pairs. The

simplified Solis clear-sky model is then applied to the apparent solar

elevation to produce global horizontal irradiance (GHI) in W m⁻².

Longitudes are normalised to the range [-180, 180] before the solar

position calculation.

Args:

times: Ordered list of timezone-aware datetimes for which to

compute clear-sky irradiance.

latitudes: 1-D array of latitude values in degrees North.

longitudes: 1-D array of longitude values in degrees East

(values > 180 are wrapped to [-180, 180]).

Returns:

xr.Dataset with a single variable keyed by

PVLIB_CLEARSKY_VARIABLE_NAME of shape (time, latitude, longitude)

containing GHI in W m⁻². The time coordinate is stored without

timezone information.

"""

A suggestion for a docstring

KristianHMoller · 2026-06-29T12:59:33Z

 ## [Unreleased]
+* Made minutes available for formatting file names, @JoachimKoenigslieb
+- use `.expand_dims` instead of `.assign_coords` to make sure we have both time dimension and time coordinates when loading from files, @JoachimKoenigslieb
+- Added a helper `make_pvlib_clearsky_dataset` which creates clearsky data by calling `pvlib`. Also added a new config variable `clearsky_source` which defaults to `file` and switches behavior when set to `pvlib`, @JoachimKoenigslieb
+- `check_solar_elevation` now does not assume location is in Copenhagen by default, @JoachimKoenigslieb
+

 ### Added

 - Added a check for the number of ensemble members, as the code currently supports only one [!13](https://github.com/dmidk/sunflow/pull/13), @KristianHMoller
 - Subsetting to bounding box is now also done in the `s3` and `files` code paths [!11](https://github.com/dmidk/sunflow/pull/11), @JoachimKoenigslieb
 - Subsetting to bounding box now correctly handles both ascending and descending lat/lon coordinates [!11](https://github.com/dmidk/sunflow/pull/11), @JoachimKoenigslieb

 ### Changed

 - Pass down number of ensembles from configurations to `ProbabilisticAdvection`. This gives a roughly 3x speedup in the no ensembles case [!12](https://github.com/dmidk/sunflow/pull/12), @JoachimKoenigslieb


Suggested change

## [Unreleased]

### Added

- Added a helper `make_pvlib_clearsky_dataset` which creates clearsky data by calling `pvlib`. Also added a new config variable `clearsky_source` which defaults to `file` and switches behavior when set to `pvlib` [!15](https://github.com/dmidk/sunflow/pull/15), @JoachimKoenigslieb

- Made minutes available for formatting file names [!15](https://github.com/dmidk/sunflow/pull/15), @JoachimKoenigslieb

- Added a check for the number of ensemble members, as the code currently supports only one [!13](https://github.com/dmidk/sunflow/pull/13), @KristianHMoller

- Subsetting to bounding box is now also done in the `s3` and `files` code paths [!11](https://github.com/dmidk/sunflow/pull/11), @JoachimKoenigslieb

- Subsetting to bounding box now correctly handles both ascending and descending lat/lon coordinates [!11](https://github.com/dmidk/sunflow/pull/11), @JoachimKoenigslieb

### Changed

- `check_solar_elevation` now does not assume location is in Copenhagen by default [!15](https://github.com/dmidk/sunflow/pull/15), @JoachimKoenigslieb

- Use `.expand_dims` instead of `.assign_coords` to make sure we have both time dimension and time coordinates when loading from files [!15](https://github.com/dmidk/sunflow/pull/15), @JoachimKoenigslieb

- Pass down number of ensembles from configurations to `ProbabilisticAdvection`. This gives a roughly 3x speedup in the no ensembles case [!12](https://github.com/dmidk/sunflow/pull/12), @JoachimKoenigslieb

I am not sure if the "suggestion" works as intended here.

KristianHMoller · 2026-06-29T13:04:31Z

All implemented functionality seems to work as intended! This is great!
I have a few suggestions of which the one regarding maximum solar elevation is scientific, whereas the others are mainly format

AdamRJensen

I left a few comments, but my main concern that I want to raise is that climatological clear sky models as used in this PR are not ideal for forecasting applications given that water vapor varies significantly from day to day. But if this is for testing purposes, then it's fine to add.

Using CAMS clear sky data based on forecasting of aerosols is the way forward in my opinion.

AdamRJensen · 2026-06-30T19:40:17Z

+        solar_position = pvlib.solarposition.get_solarposition(
+            repeated_times,
+            flat_latitudes,
+            flat_longitudes,


Suggested change

flat_longitudes,

flat_longitudes,

method='nrel_numpy',

I suggest explicitly denoting which solar position model is used.

On a different note, the 'nrel_numpy' model is the most accurate but unnecessarily slow. Based on my recent research, solar position models such as Michalsky or USNO are plenty fine: https://solposx.readthedocs.io/. How fast they are is very implementation dependent, and most of these implementations are not developed for a grid of latitudes/longitudes.

AdamRJensen · 2026-06-30T19:43:26Z

+- Added a helper `make_pvlib_clearsky_dataset` which creates clearsky data by calling `pvlib`. Also added a new config variable `clearsky_source` which defaults to `file` and switches behavior when set to `pvlib`, @JoachimKoenigslieb
+- `check_solar_elevation` now does not assume location is in Copenhagen by default, @JoachimKoenigslieb


pvlib supports a lot of clear sky models, even remotely retrieving clear sky data from CAMS. I recommend being explicit here about which method is being used.

Joachim Kønigslieb added 3 commits June 15, 2026 14:35

Add smaller fixes to make running from files more robust

8c3f3ea

add changelog

753864e

check all corners instead of the central point of the domain so we do…

59a9af1

…nt skip out on forecasts on the borders of the domain near sunset/sunrise

KristianHMoller self-requested a review June 26, 2026 09:01

KristianHMoller reviewed Jun 26, 2026

View reviewed changes

KristianHMoller reviewed Jun 29, 2026

View reviewed changes

AdamRJensen reviewed Jun 30, 2026

View reviewed changes


		from .geospatial import get_coordinates

		PVLIB_CLEARSKY_VARIABLE_NAME = "pvlib_clearsky_ghi"

	repeated_times = pd.DatetimeIndex([time] * len(flat_latitudes))
	repeated_times = [time] * len(flat_latitudes)

-) -> xr.Dataset:
+) -> xr.Dataset:
+    """Compute a pvlib simplified-Solis clear-sky GHI dataset on a lat/lon grid.
+    For each timestep in `times`, solar position is computed at every grid
+    point by repeating the timestamp across all (lat, lon) pairs. The
+    simplified Solis clear-sky model is then applied to the apparent solar
+    elevation to produce global horizontal irradiance (GHI) in W m⁻².
+    Longitudes are normalised to the range [-180, 180] before the solar
+    position calculation.
+    Args:
+        times: Ordered list of timezone-aware datetimes for which to
+            compute clear-sky irradiance.
+        latitudes: 1-D array of latitude values in degrees North.
+        longitudes: 1-D array of longitude values in degrees East
+            (values > 180 are wrapped to [-180, 180]).
+    Returns:
+        xr.Dataset with a single variable keyed by
+        PVLIB_CLEARSKY_VARIABLE_NAME of shape (time, latitude, longitude)
+        containing GHI in W m⁻². The time coordinate is stored without
+        timezone information.
+    """

		- Added a helper `make_pvlib_clearsky_dataset` which creates clearsky data by calling `pvlib`. Also added a new config variable `clearsky_source` which defaults to `file` and switches behavior when set to `pvlib`, @JoachimKoenigslieb
		- `check_solar_elevation` now does not assume location is in Copenhagen by default, @JoachimKoenigslieb

Uh oh!

Conversation

JoachimKoenigslieb commented Jun 15, 2026

Uh oh!

KristianHMoller commented Jun 26, 2026

Uh oh!

KristianHMoller commented Jun 26, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoachimKoenigslieb Jun 26, 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!

KristianHMoller commented Jun 29, 2026

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!

KristianHMoller commented Jun 29, 2026

Uh oh!

AdamRJensen left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JoachimKoenigslieb Jun 26, 2026 •

edited

Loading