Riksrevisjonen / pioneeR

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

Add custom functions for DEA calculations #82

Closed Aeilert closed 6 months ago

Aeilert commented 7 months ago

Add custom functions for efficiency, super efficiency, slack and peer calcuations. This removes Benchmarking as a hard dependency.

The commit implies some minor changes in the slack table shown in the Analyse tab, but should otherwise not effect the UI.

Includes:

Closes #80

ohjakobsen commented 6 months ago

Slack differs quite significantly from Benchmarking on some of the datasets I have available. Using the Electric_plants data frame from deaR, slack differs with the following parameters:

Inputs: Labor, Fuel, Capital Outpus: Output RTS: Variable Orientation: Input

Some firms differ with RTS set to NIRS, and they differ quite significantly with NDRS.

Interestingly, the pear package gives the same results as Benchmarking package, so the difference lies somewhere in the implementation in pioneeR.

ohjakobsen commented 6 months ago

Here is a reproducible example. The compute_slack() function in pioneeR (and in pear) will give the same results as Benchmarking, but the implementation within the app itself is flawed. It is possible to use model arguments in the slack function that differs from the original DEA model. This is what happens in the app, as the default values for CRS and input orientation will always be selected.

df <- deaR::Electric_plants

X <- cbind(df$Labor, df$Fuel, df$Capital)
Y <- as.matrix(df$Output)

vrs_pioneer <- pioneeR::compute_efficiency(X, Y, type = 'vrs', orientation = 'in', digits = NULL)
vrs_pear <- pear::compute_efficiency(X, Y, type = 'vrs', orientation = 'in', digits = NULL)
vrs_benchmarking <- Benchmarking::dea(X, Y, RTS = 'vrs', ORIENTATION = 'in')

slack_pioneer <- pioneeR::compute_slack(X, Y, vrs_pioneer$unadj_values, type = 'vrs', orientation = 'in', digits = NULL)
slack_pear <- pear::compute_slack(X, Y, vrs_pear$unadj_values, type = 'vrs', orientation = 'in', digits = NULL)
slack_benchmarking <- Benchmarking::slack(X, Y, vrs_benchmarking)

# Same results in pioneeR, pear and Benchmarking
slack_pioneer$values
slack_pear$values
slack_benchmarking$sum

Reproduce incorrect results in the current PR:

slack_pear2 <- pear::compute_slack(X, Y, vrs_pear$unadj_values, type = 'crs', orientation = 'in', digits = NULL)

# Gives wrong results as returns to scale is CRS
slack_pear2$values
Aeilert commented 6 months ago

@ohjakobsen : The specific bug in the UI should now be fixed.

ohjakobsen commented 6 months ago

@ohjakobsen : The specific bug in the UI should now be fixed.

This fixes the bug, but the implementation is still problematic, and these are exported functions that will be available to the user. I will do a full review and request changes to the implementation.

Aeilert commented 6 months ago

@ohjakobsen : The specific bug in the UI should now be fixed.

This fixes the bug, but the implementation is still problematic, and these are exported functions that will be available to the user. I will do a full review and request changes to the implementation.

Sounds good. I agree there is no explicit need to export these functions. I think the simplest solution is to use @keywords internal in documentation, but other options and code changes could also be valid.

Aeilert commented 6 months ago

Several changes should be done to this PR:

  • Reduce the number of exported functions for now. The user needs a function that returns the DEA model, slack and super efficiency (non exported functions should have @noRd)
  • The compute_efficiency() function should return an S3 object that has a custom print method (with a digits argument). This object can be passed to further functions
  • inst/app/R/fct_dea_boostrap.R must be updated as well (and change the rather embarrassing typo in the filename)
  • The code includes a lot of commented code that should be removed before we merge
  • Adhere to style guide for functions, in particular on the use of return

Hi @ohjakobsen,

Thanks for the review. As discussed offline, I have added several improvements and changes to this PR. The nature of the suggested changes required a more comprehensive approach. The good news is that the end-result is quite a lot better. :)

Important changes include:

TO DO:

Suggestion to the report:

# Estimate the DEA model
model  <-  compute_dea(
  df,
  id = idvar,
  input = inputs
  output = outputs, 
  type = rts, 
  orientation = orientation,
  super = TRUE,
  slack = TRUE,
)

# Print out the summary of the DEA model
summary_tbl_dea(model)  

# Summary statistics for the efficiency scores
summary(model$results$efficiency)

# Get table for scale efficiency
compute_scale_efficiency(inputs, outputs, orientation, digits = 4L)
Aeilert commented 6 months ago

An additional comment on dep. warnings for old functions. I think it is correct to add a dep. warning for create_matrix() since it basically is no longer in use at all (when also removed from the report). But when running pioneeR::run_pioneer() this might look weird.

Its just every 8 hours though, so I think we can live with this

image

ohjakobsen commented 6 months ago

An additional comment on dep. warnings for old functions. I think it is correct to add a dep. warning for create_matrix() since it basically is no longer in use at all (when also removed from the report). But when running pioneeR::run_pioneer() this might look weird.

Its just every 8 hours though, so I think we can live with this

image

Actually, we should rethink this. create_matrix() was only included as a helper function for the 0.3.0. We should not confuse the user with irrelevant warnings that the user cannot do anything about. We should either postpone the deprecation, or make sure that the warning is not shown when the user runs run_pioneer().

Edit: I will remove the deprecation. It cannot be deprecated before we remove it from server.R, and it should not be removed from there yet. This PR already does too much, so will fix it later on.

ohjakobsen commented 6 months ago

Updated this PR with the following changes:

Aeilert commented 6 months ago

Thanks @ohjakobsen

I reviewed the recent changes. They look good!