Riksrevisjonen / pioneeR

R package for running a Shiny app for DEA analysis
https://riksrevisjonen.github.io/pioneeR/
6 stars 2 forks source link

Add major improvments to Malmquist #106

Closed Aeilert closed 4 months ago

Aeilert commented 5 months ago

Closes #81

Aeilert commented 4 months ago

Thanks for the review @ohjakobsen. Some comments:

  • Add reference year to the table (or make sure ref.year is always t-1 and let this be known in the UI)

I'm not sure we need both time and ref.time in the table. For the current implementation ref.time is always t-1, which is how Malmquist is normally calculated and presented. We can explain this, together with the interpretations, in the report. And yes; it is theoretically possible for ref.time to be something else then t-1 (e.g using the same base-year for all comparisons), but that's something we can deal with if this is implemented at a later stage. I did however add a simple banner that might help clarify things. E.g. it is probably a good idea to highlight the results are now presented a la Farrell.

  • Output table is quite noisy. Several of the columns should be optional. Add ability to toggle on/off under table options

I agree. I added some changes to how the table is presented by 1) adding a toggle option for showing all sub-components and 2) using reactable options more explicitly. How tables and columns are shown in the different tabs is not always consistent, so an overall review of this is probably a good idea (including a utility function for presenting common default columns as you suggest).

  • Add option to output pretty column names in export (at least for Excel)

The standard behaviour for other exports seems to be that results are always returned with pretty column names. I have change the code in this PR to align with this. We can restructure and add toggle options in a later PR.