Support global models - #204
Conversation
| ] | ||
| if shp and shp != [""]: | ||
| has_shapefiles = bool(shp and shp != [""]) | ||
| if has_shapefiles: |
There was a problem hiding this comment.
If I understand correctly, now as soon as there is a shapefile in the regions, there is a "hardcoded" all region that is the ICON domain. If no shapefile is passed, the behaviour changes for regional models, where the results are going to be global.
Potential issues I see:
- Would we ever pass other shapefiles that are not within our hardcoded
all? - We should make it clear that for regional forecaster, not passing any region shapefile changes the reulsts.
What do you think?
There was a problem hiding this comment.
correct and I introduced it mostly for backward compatibility. But maybe we can try to find a better logic here? @jonasbhend is also refining the hardcoded domain as part of #191
There was a problem hiding this comment.
But my point is that it's not backward compatible, or am I missing something?
What was the expected domain of verification if no shapefile was passed? With this PR, it is a global verification. I thought that it was always regional so far.
There was a problem hiding this comment.
it's backward compatible to the extent that we usually include regions for stratification, but otherwise no, results will change for an empty lists of regions
There was a problem hiding this comment.
What is the intended behavior for global models? Verify on the globe?
Couldn't we check the list of regions againts the data domain and only iterate on those that are fully within the data domain? That way, we could use a different set of shapefiles for global and regional analysis. Not sure how to elegantly handle the different defaults for global (take all the data) and regional (take the inner 'all' domain) though. I guess with a simple conditional should work for now.
There was a problem hiding this comment.
Is the domain selection now as you want it? I think we are close to a merge. Let me know if there is still something to be done
There was a problem hiding this comment.
I would still argue that we should take this opportunity to harmonize the region definitions and make regions explicit. I.e. avoid the all region that will be the icon domain or global domain depending on the use case. Instead I propose to use:
- default to
icondomain if no stratification is provided - use
globalfor the global verification to avoid confusion
This of course implies that we have to select the icon domain by default in the dashboard and plots.
There was a problem hiding this comment.
What do you think about avoiding defaults and having the schema expect an explicit value for the region of interest?
There was a problem hiding this comment.
Here's another proposal:
- we disallow region = None and make selection of regions explicit (i.e. we can use icon and global as names)
- first region gets plotted and used in dashboard
There was a problem hiding this comment.
commit 69099e2 implements your suggestion, can you have a look?
…r cloud cover Co-authored-by: clairemerker <34312518+clairemerker@users.noreply.github.com>
jonasbhend
left a comment
jonasbhend
left a comment
There was a problem hiding this comment.
I have a few minor comments, otherwise looking good. Sorry for taking so long.
| # TODO: ssrd is treated as a plain per-step field (no de-accumulation), | ||
| # which only holds because it's not currently listed in any | ||
| # accumulate_from_start_of_forecast.accumulations in the inference | ||
| # configs (unlike tp, see _tot_prec_handling). If ssrd/strd are ever | ||
| # added there, this needs the same cumulative-since-start handling tp | ||
| # gets, or verification/plots will silently be wrong. |
There was a problem hiding this comment.
So we already produce this, but in fact it is accumulated (or not)? I am confused...
There was a problem hiding this comment.
just for my enlightenment, not really important.
There was a problem hiding this comment.
my understanding is that we predict the same variable used during training, which is period accumulated
There was a problem hiding this comment.
Sorry missed the comment.
We produce it only on some global experiments are the moment. The unit is W/m^2, and Watts are depending on a time of accumulation, so it's similar to precipitation in that way, and it could be produced with the accumulation from the start of the forecast just like precip. For simplicity now I treated is as an instantaneous variable (and the accumulation depends on the dataset used, 1h or 6h)
thanks! I addressed your comments above, hopefully a final look? |
4 participants
supersedes #90 following #190
additionally: