arturoesquerra / geolift Goto Github PK

Hi Arturo.

Thanks for this awesome package. We (Nico and I) have already shared with you some of these feedbacks and bugs but I'll post them here so we can do follow up:

• GeoLiftData class object to prevent input errors and less inputs, created when running GeoDataRead (or equivalent)
• Apply TrimControls automatically (or warn user) when “too many cities” logic applies: (QT * 5 > QC) or if we already have nloc results as vector N. When too many cities are passed, the power calculations last ages.

if (max(N) * 5 > length(unique(GeoTestData$location)))
  GeoTestData <- TrimControls(GeoTestData)

• Add @examples into the documentation for every function.
• Status bar for functions with n simulations. Reference.
• BUG: Redundant combinations on GeoLiftPower.search results
• Consistency on function default arguments (i.e. “Units” in GeoLiftPower.search and “Y” on the rest for the same variable)
• augsynth library issue (changes without version change) [Force user to use version ebenmichael/augsynth@04daba1]
• The printed object shown in NumberLocations is not 100% usable on the pipeline (it is printed but not returned). What is the relevant outcome to return?
• BUG: NumberLocations breaks when trying more than 1 treatment_periods. Is it possible to fix this by generating a list for each treatment period?
• What should the user do when running GeoLiftPower.search and the following message is printed: The treatment is larger than 80% of all available data.
• Suppress plyr dependency (not used + conflict with dplyr)
• Please, do not call attachNamespace inside functions. Functions have already been imported and when doing this in some servers, it will return errors instead of skipping.
• BUG: GeoDataRead: Return error message when time_increments value are not >1 (user inputs only 1 day). This function’s code is poor and extensive; also, you may use lubridate functions instead.. already solves the dates logic. Even better if you force users to input time class vectors.
• Avoid forced prints. Sometimes users do not want anything printed. For example, here.
• GeoPlot should return the plot object, not print it. Use can't custom plot if the object is not returned.
• Standard plots to have the same style and object type (ggplot2).
• Plot multiple cities in single plot object (when using test_locs = TRUE) might be friendlier. You may use ggplot2::facet_grid or patchwork library or similar.
• Include market size data (% conversions) for each city combination on GeoLiftPower.search‘s output to give the user control size dimension.
• TrimControls errors should be errors, not prints.
• utils::globalVariables can be suppressed using .data$
• No README.md file?
• There are some arguments that the user may set but are hardcoded inside the function so those won’t make any difference
• 2 typos here (Tt and than)
• BUG: 1 single location in treatment is not allowed. Error in 1:nrow(BestMarkets_aux[, 1:1]): argument of length 0

CC: @nicocru