Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor steps in blending code #440

Open
sidekock opened this issue Nov 15, 2024 · 3 comments
Open

Refactor steps in blending code #440

sidekock opened this issue Nov 15, 2024 · 3 comments
Assignees
Labels
enhancement New feature or request

Comments

@sidekock
Copy link
Contributor

Issue #395 should be done for the blending code aswel.

@sidekock sidekock self-assigned this Nov 15, 2024
@sidekock sidekock added the enhancement New feature or request label Nov 18, 2024
@sidekock
Copy link
Contributor Author

I have started with the rough outlines of the code but will work on it later this week.

@sidekock
Copy link
Contributor Author

@mats-knmi I have a question regarding the ndim=4 check you did to determine if the forecast provided has been decomposed already or not. I see a lot of potential for problems here. Let me try and list my assumptions and downsides below, I would love to hear your thoughts on possible solutions:

  • I think the 4 dims is due to the assumption (member, timesteps, x, y).
  • But what in the case someone has a single member they want to blend, then (timesteps, cascade levels, x, y) is also 4D
  • Do you think an extra flag in the config data class would be the way to go? If so, what do we assume to be the default value? (I would say, decomposed as a basis as this is not introducing breaking changes)
    Would love to hear your feedback on this.

@mats-knmi
Copy link
Contributor

I didn't create this check, it was already in the original code, I just moved it around. There is a line in the docstring telling users to add a dimension in the case you describe.

I do agree that this is less then ideal. In my opinion the ideal solution would involve xarray, because in the case of an xarray dataset all dimensions would be named and thus you wouldn't have to assume anything based on the number of dimensions. You could do something like if "member" in dataset:.

The xarray conversion I have been working on has been on hold for a bit, since I have been busy with other things, but the nowcasting code had already been entirely converted to xarray and I have merged your refactored code in there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: No status
Development

When branches are created from issues, their pull requests are automatically linked.

2 participants