Skip to content

Unify regions stations#154

Open
cosunae wants to merge 37 commits into
mainfrom
unify_regions_stations
Open

Unify regions stations#154
cosunae wants to merge 37 commits into
mainfrom
unify_regions_stations

Conversation

@cosunae
Copy link
Copy Markdown
Contributor

@cosunae cosunae commented May 18, 2026

When calling the showcase with a number of regions (for the plots) and locations for meteograms, currently we launch 1 task per region and location. While this maximizes parallelism, it is not efficient given these tasks are mostly IO bound that they will load the same data (entire horizontal plane) multiples times in order to select later on the region/location.

  params:
    - T_2M
    - SP_10M
    - TOT_PREC
  meteograms: false
  animations: true
  regions:
    - europe
    - switzerland
    - name: alpine_arc
      extent: [-16.0, 25.0, 30.0, 65.0]
      projection: orthographic

  stations: [JUN, COV, GOR, WFJ, SAE, SAM, DAV, ZER, ANT, VSBAS, BRT, LTB, GOS, CEV, BIA]

In this PR we modify the following:

  • snakemake rule so that the plot_meteogram and plot_forecast_frame generate a list of plots (1 per region) and meteograms (1 per location)
  • move the marimo files into a pure python script. This I would propose in order to make the code more readable in a pure VS code (not using marimo editor) and more homogeneous codebase (not mixing marimo and python). I checked with @dnerini, but let me know if you do not think this is a good change.
  • adapt the python scripts to process multiple regions and select multiple locations.

Louis-Frey and others added 27 commits April 22, 2026 09:05
Co-authored-by: Francesco Zanetta <62377868+frazane@users.noreply.github.com>
Co-authored-by: Michele Cattaneo <44707621+MicheleCattaneo@users.noreply.github.com>
Co-authored-by: Hugues de Laroussilhe <hugues.delaroussilhe@meteoswiss.ch>
Co-authored-by: Daniele Nerini <daniele.nerini@meteoswiss.ch>
Co-authored-by: Daniele Nerini <daniele.nerini@meteoswiss.ch>
@cosunae cosunae marked this pull request as draft May 18, 2026 09:26
@cosunae cosunae mentioned this pull request May 18, 2026
@cosunae cosunae marked this pull request as ready for review May 18, 2026 14:02
Copy link
Copy Markdown
Contributor

@frazane frazane left a comment

Choose a reason for hiding this comment

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

Very nice. Fully support moving away from marimo notebooks here, I don't think they fit the use case. We might bring them back in the future for other more interactive things.

Comment thread workflow/scripts/plot_meteogram.py
Comment thread workflow/scripts/plot_forecast_frame.py
Comment thread workflow/scripts/plot_forecast_frame.py Outdated
}


try:
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The try-except logic is duplicated

Comment thread workflow/rules/plot.smk
@jonasbhend
Copy link
Copy Markdown
Contributor

Hi @cosunae Thanks for pushing this. Could you maybe rearrange the commits so that git(hub) understands that the files have been renamed? Otherwise it's impossible to identify changes made to the scripts. I could give you a hand if need be, just let me know (don't want to interfere with your work, as the fix will rewrite history).

@cosunae
Copy link
Copy Markdown
Contributor Author

cosunae commented May 19, 2026

Hi @cosunae Thanks for pushing this. Could you maybe rearrange the commits so that git(hub) understands that the files have been renamed? Otherwise it's impossible to identify changes made to the scripts. I could give you a hand if need be, just let me know (don't want to interfere with your work, as the fix will rewrite history).

I will try, yes, but I think it wont help with the review because the files are very different (Inherently because the structure of a marimo and python file are).
I think In principle git should figure out these files are the same, but in this case it does not work because the diff is too large

@cosunae cosunae force-pushed the unify_regions_stations branch from 3d9465f to c3775ad Compare May 19, 2026 09:58
@cosunae cosunae force-pushed the unify_regions_stations branch from c3775ad to c1f758b Compare May 19, 2026 10:03
@cosunae
Copy link
Copy Markdown
Contributor Author

cosunae commented May 19, 2026

done, after rewriting the git history with a git mv and a force push, still similarity is very small, so PR will refuse to show it as a diff

@jonasbhend
Copy link
Copy Markdown
Contributor

done, after rewriting the git history with a git mv and a force push, still similarity is very small, so PR will refuse to show it as a diff

Thanks and sorry for putting you on this. I didn't think about the python script / marimo issue

@cosunae cosunae changed the base branch from general_meteogram to main May 19, 2026 15:48
@cosunae cosunae changed the base branch from main to general_meteogram May 19, 2026 15:49
Base automatically changed from general_meteogram to main May 22, 2026 07:53
@cosunae
Copy link
Copy Markdown
Contributor Author

cosunae commented May 27, 2026

anything missing here @frazane @jonasbhend ? could you take another look?

Copy link
Copy Markdown
Contributor

@jonasbhend jonasbhend left a comment

Choose a reason for hiding this comment

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

Hi @cosunae, thanks for tackling this. It is looking good from what I can tell. However, when trying to run the interpolator config with meteograms enabled, I get an error. This is not coming from your PR, as the error is already in main (see below). Is this something we want to fix as part of this PR?

evalml showcase config/interpolators-ich1.yaml -n
host: balfrin-ln002
Building DAG of jobs...
InputFunctionException in rule plot_meteogram in file "/scratch/mch/bhendj/evalml-opr/workflow/rules/plot.smk", line 28:
Error:
  ValueError: No baseline zarr found for init time 202503010000
Wildcards:
  showcase=20260527_interpolators-ich1_7de8
  run_id=interpolator-tmp-d5aa-on-forecaster-c304-1e7e/7d5c
  init_time=202503010000
  param=T_2M
Traceback:
  File "/scratch/mch/bhendj/evalml-opr/workflow/rules/plot.smk", line 53, in <lambda>
  File "/scratch/mch/bhendj/evalml-opr/workflow/rules/plot.smk", line 24, in _get_available_baselines (rule plot_meteogram, line 30, /scratch/mch/bhendj/evalml-opr/workflow/rules/plot.smk)

@jonasbhend
Copy link
Copy Markdown
Contributor

Hi @cosunae, thanks for tackling this. It is looking good from what I can tell. However, when trying to run the interpolator config with meteograms enabled, I get an error. This is not coming from your PR, as the error is already in main (see below). Is this something we want to fix as part of this PR?

evalml showcase config/interpolators-ich1.yaml -n
host: balfrin-ln002
Building DAG of jobs...
InputFunctionException in rule plot_meteogram in file "/scratch/mch/bhendj/evalml-opr/workflow/rules/plot.smk", line 28:
Error:
  ValueError: No baseline zarr found for init time 202503010000
Wildcards:
  showcase=20260527_interpolators-ich1_7de8
  run_id=interpolator-tmp-d5aa-on-forecaster-c304-1e7e/7d5c
  init_time=202503010000
  param=T_2M
Traceback:
  File "/scratch/mch/bhendj/evalml-opr/workflow/rules/plot.smk", line 53, in <lambda>
  File "/scratch/mch/bhendj/evalml-opr/workflow/rules/plot.smk", line 24, in _get_available_baselines (rule plot_meteogram, line 30, /scratch/mch/bhendj/evalml-opr/workflow/rules/plot.smk)

I'll try to fix this

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.

4 participants