RMI-PACTA / pacta.multi.loanbook.plot

Tools to Plot Climate Metrics for Multiple Loanbooks
https://rmi-pacta.github.io/pacta.multi.loanbook.plot/
Other
0 stars 0 forks source link

Adjust to variable result grouping #30

Closed jacobvjk closed 4 months ago

jacobvjk commented 5 months ago

depends on https://github.com/RMI-PACTA/pacta.multi.loanbook.analysis/pull/34 closes #27 closes #25 by coincidence

jacobvjk commented 5 months ago

@jdhoffa @MonikaFu I am still keeping this in draft mode, as we probably want to agree on the linked PR first. But it may make sense to take a brief look at this PR now as well. Explanation is above, but just a quick tl;dr:

the aggregate metric needs to be flexible for analysis, allowing slicing and dicing of the loan books by user defined variables (e.g. by banks, but possibly also by membership in alliances, loan structures, etc). The prep and plot functions should cover this in a basic way but could probably use some prettification. open to ideas and suggestions here

jdhoffa commented 5 months ago

Ah this might be a bit of an anti-pattern.

Specifically for the prep_ and plot_ functions, I would tend to prefer that they always expect a single unit of input to prep or plot (so as to not overcomplicate them). And if iterating over groups is desired, I would imagine doing that either in:

is probably desired?

I don't think hard-baking the grouping functionality into the plotting functions is a good idea...

jacobvjk commented 4 months ago

Ah this might be a bit of an anti-pattern.

Specifically for the prep_ and plot_ functions, I would tend to prefer that they always expect a single unit of input to prep or plot (so as to not overcomplicate them). And if iterating over groups is desired, I would imagine doing that either in:

  • theworkflow.*
  • some wrapper function
  • a vignette

is probably desired?

I don't think hard-baking the grouping functionality into the plotting functions is a good idea...

It's definitely not pretty at this point and it may be worth reviewing which of the plots realisitically need to show any group information. But I think in some cases we explicitly want the group info inside the plot. See e.g. https://github.com/RMI-PACTA/workflow.aggregate.loanbooks/pull/95

more generally: the group will - at least in most cases - be used as an aesthetic (in ggplot terms). So iterating over the groups will likely not do it

jdhoffa commented 4 months ago

Ok got it, then totally fair enough!