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.

We have seen some usefulness of forecasting very close to sunrise/sundown actually. I will modify this to be a config 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: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: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.

Sounds like a good suggestion. Will be more concrete about which method is used.

I agree that getting clearsky values from CAMS is probably the best we can do. But it also adds a bit more complexity, since we need to make sure that the CAMS forecast is available when the nowcast should be produced.

For my use case I think I would be happy with getting any approximately "okay" clearsky value

Co-authored-by: KristianHMoller <150110122+KristianHMoller@users.noreply.github.com>

Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>

JoachimKoenigslieb · 2026-07-07T11:56:24Z

Have been a bit swamped with other things but have not forgotten about this MR!

Will apply all suggestions and get it ready.

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

Comment thread sunflow/forecast.py Outdated

KristianHMoller reviewed Jun 29, 2026

View reviewed changes

Comment thread sunflow/forecast.py

KristianHMoller reviewed Jun 29, 2026

View reviewed changes

AdamRJensen reviewed Jun 30, 2026

View reviewed changes

JoachimKoenigslieb and others added 3 commits July 7, 2026 13:46

Update sunflow/forecast.py

d8d2828

Co-authored-by: KristianHMoller <150110122+KristianHMoller@users.noreply.github.com>

Update sunflow/forecast.py

7dd6081

Co-authored-by: KristianHMoller <150110122+KristianHMoller@users.noreply.github.com>

Update sunflow/forecast.py

1bfa47d

Co-authored-by: Adam R. Jensen <39184289+AdamRJensen@users.noreply.github.com>


		from .geospatial import get_coordinates

		PVLIB_CLEARSKY_VARIABLE_NAME = "pvlib_clearsky_ghi"

		- 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!

Choose a reason for hiding this comment

Uh oh!

KristianHMoller commented Jun 29, 2026

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoachimKoenigslieb commented Jul 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JoachimKoenigslieb Jun 26, 2026 •

edited

Loading