Merged
Conversation
Contributor
Author
|
I actually need not this be merged to run it on irithree and share with partners. So you can take your time to review. |
Contributor
Author
|
Do we want to merge this nonetheless? Or I keep on piling up new commits here for optimization we just discussed? |
Contributor
Author
|
@xchourio please review as Aaron will be left out of calculations PRs in the future. I think we should review each other because in case one leaves IRI before the other, we keep familiar with the other's work and can pick up more easily. In that vein, you might want to start having me PR-review your PepsiCo work around the crop app. |
xchourio
approved these changes
Jan 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a first working version of the new app for PepsiCo. Very much like the previous one, but now instead of doing seasonal averages from monthly data for all variables of the models, it derives seasonal quantities from the daily.
For now there are only wwc quantities that I figured needed not a dedicated function. Those that do are currently under PR review and will be added later, using the map method, as was done for seasonal onset. Quantiles and Frost Duration are written up here but removed from the list of options in the layout because, respectively: quantiles seem to throw off the app and Frost Duration is just a draft (I didn't expect the flipping of indices of the groups to work just like that, and they didn't).
Note that while daily_tobegroupedby_seasons is a copy and paste from enacts, I did change it so that the seasons_starts/ends be coords rather than vars. Compared to how the previous app is working, I did have also to make sure that the inputs of daily_tobegroupbed_by be xr.Dataset and not xr.DataArray. In the other app, it was ok to have either (ds for local plots and array for map) but not here.
So the app now produces two links for each app, as a multi-page app. Note also that that layout file is greatly reduced by using the recently merged layout utilities. I can see that I might be a bit inconsistent in whether I write a function in maproom or app_calc. Will potentially reconsider when/if factoring out.
Note finally that some daily data were centered on noon while others on midnight. This threw me off for a moment before I figure that out. It probably made no difference when creating the monthly version of the zarr storages.
I need to show something for the 15th, so I guess Friday I need a version of this to run in irithree.
I left a few comments in line for things that seem to be out of whack, or as reminders of next steps. In particular next PRs should: