ibarraespinosa commented 7 months ago

Hi,

https://github.com/openjournals/joss-reviews/issues/5975

I'm sorry it took me ages to start review the repo and manuscript. Hopefully, as already was reviewewd once, it will be a smooth process.

I will be comenting here all my doubts and questions and once they al solved or clarified it is done.

ibarraespinosa commented 7 months ago

first example does not work


> # examples
> ## -- load gcamreport library.
> library(gcamreport)
> ## -- store the database path, name, and scenarios in a variable.
> dbpath <- "examples"
> dbname <- "database_basexdb_ref"
> scen <- "Reference"
> ## -- choose a project name
> prjname <- "example1.dat"
> ## -- generate the reporting dataset until 2050 for EU-12 and EU-15 for all the 
> ## -- Agricultural variables, save the output in .RData, .csv and .xlsx format, 
> ## -- and lunch the user interface
> generate_report(db_path = dbpath, db_name = dbname, scenarios = scen, 
+                 prj_name = prjname, final_year = 2050, 
+                 desired_regions = c('EU-12', 'EU-15'), 
+                 desired_variables = c('Agricultural*'), 
+                 save_output = TRUE, launch_ui = TRUE)
[1] "Creating project..."
/home/sergio/basex/.basex: writing new configuration file.
Error in localDBConn(db_path, db_name, migabble = FALSE) : 
  Database does not exist or is invalid: examples/database_basexdb_ref
In addition: Warning messages:
1: In normalizePath(dbPath) :
  path[1]="examples": No such file or directory
2: The following named parsers don't match the column names: name, date, version

ibarraespinosa commented 7 months ago

shiny needs to be added in imports or depends in DESCRIPTION

ibarraespinosa commented 7 months ago

launch_gcamreport_ui("examples/example3.RData")

Listening on http://127.0.0.1:7278

Attaching package: ‘magrittr’

The following objects are masked from ‘package:testthat’:

equals, is_less_than, not

Attaching package: ‘shinydashboard’

The following object is masked from ‘package:graphics’:

box

Learn different usages for shinyjs and other Shiny tricks: https://deanattali.com/blog/advanced-shiny-tips

Attaching package: ‘shinyjs’

The following object is masked from ‘package:shinyWidgets’:

alert

The following object is masked from ‘package:shiny’:

runExample

The following objects are masked from ‘package:methods’:

removeClass, show

Warning in file(con, "r") : cannot open file '/home/sergio/R/www/style.css': No such file or directory Warning: Error in file: cannot open the connection 72: file 71: readLines 70: includeCSS 2: runApp 1: launch_gcamreport_ui [/home/sergio/gcamreport/R/main.R#724]

klau506 commented 7 months ago

Dear @ibarraespinosa,

Thank you for your comments and performed validations. Find attached some details:

Regarding the First example: it might be that either you did not clone the repository, you are not using the gcamreport project, or you do not have placed the example database in the correct folder. It might also be that, when extracting the database from the zip folder, an intermediate folder appeared and the database is located into examples/database_basexdb_ref/database_basexdb_ref instead of examples/database_basexdb_ref. I have written a few lines in the tutorials to make sure no one experiences this error.
I have checked, and shiny is already present in the DESCRIPTION file.
Regarding the Third example: it might be that either you did not clone the repository or you are not using the gcamreport project. I have written a few lines in the tutorials to make sure no one experiences this error.

In order to launch the shiny app, the package relies on some files (e.g., css files) that were difficult to include as package data. Thus, the repository clone is necessary to experience the full capabilities of the tool.

Thank you for your time and hope that the clarifications are okay,

Best wishes, Clàudia

ibarraespinosa commented 6 months ago

Hi, I was able to download the input data and generate the gui.

I have a very basic question. Brazil should be one available region, however, it does not appear in the report


## -- load gcamreport library.
devtools::load_all()

## -- store the database path, name, and scenarios in a variable.
dbpath <- "examples"
dbname <- "database_basexdb_ref"
scen <- "Reference"

## -- choose a project name
prjname <- "example1.dat"

## -- generate the reporting dataset until 2050 for EU-12 and EU-15 for all the
## -- Agricultural variables, save the output in .RData, .csv and .xlsx format,
## -- and lunch the user interface

available_regions()
available_continents()

generate_report(db_path = dbpath, db_name = dbname, scenarios = scen,
                prj_name = prjname, final_year = 2050,
                desired_regions = c('Brazil', 'EU-15'),
                desired_variables = c('Agricultural*'),
                save_output = TRUE, launch_ui = TRUE)

klau506 commented 6 months ago

Dear @ibarraespinosa,

Thank you so much for this check. I reproduced the error and found that the cache was not being properly cleaned. I amended it and now works fine, regardless of your previous gcamreport runs.

Best, Clàudia

klau506 commented 6 months ago

Dear @ibarraespinosa ,

Thank you for your time and effort. Your thoughtful feedback has provided valuable insights that will undoubtedly enhance the quality of the work. After addressing all comments I would like to know if you have more suggestions or aspects that should be improved or discussed.

Keep in touch, Best wishes, Clàudia

ibarraespinosa commented 6 months ago

I will check tonight

On Fri, Mar 22, 2024 at 4:00 AM Clàudia Rodés Bachs < @.***> wrote:

Dear @ibarraespinosa https://github.com/ibarraespinosa ,

Thank you for your time and effort. Your thoughtful feedback has provided valuable insights that will undoubtedly enhance the quality of the work. After addressing all comments I would like to know if you have more suggestions or aspects that should be improved or discussed.

Keep in touch, Best wishes, Clàudia

— Reply to this email directly, view it on GitHub https://github.com/bc3LC/gcamreport/issues/6#issuecomment-2014743243, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGRM74AGZBVITZMZW745T43YZP6MXAVCNFSM6AAAAABDP5NWRWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMJUG42DGMRUGM . You are receiving this because you were mentioned.Message ID: @.***>

ibarraespinosa commented 6 months ago

Hola,

El paquete funciona muy bien y has hecho un gran trabajo. El software lo dejare por aprobado pero tengo un comentario general. Veo en DESCRIPTION que importas un gran numero de librerias. Tambien veo que importas data.table solo para retornar un objeto de clase data.table sin usar sus capacidades en operaciones. En pocas palabras, ve la forma de reducir los paquetes.

Tambien veo que en documentation escrves importfrom data.table y luego en la funcion, as.data.table, sin data.table::as.data.table. Recuerdo que la segunda forma es la apropiada. Puedes comentar por favor?

ibarraespinosa commented 6 months ago

DESCRIPTION mentiones LICENCE Mit + File but LICENSE file says BSD 2-Clause License. Is that equivalent with MIT?

ibarraespinosa commented 6 months ago

As I recall, you need to have a directory named paper in your repo with the paper in it, probably built with rticles.

ibarraespinosa commented 6 months ago

-Paper:

First phrase is too long (L9-12).
Code in summary is not needed. To that end, use the code Functionality section, where it can be a subsection or so.
What is the reference for gcamextractor?
Figure 2 does not provide much information. A flow diagram would be more helpful for new users. (suggestion)

ibarraespinosa commented 6 months ago

I could not access the help index, is that OK?

klau506 commented 6 months ago

Dear @ibarraespinosa ,

Thank you for your checks! Some answers below:

Hola,

El paquete funciona muy bien y has hecho un gran trabajo. El software lo dejare por aprobado pero tengo un comentario general. Veo en DESCRIPTION que importas un gran numero de librerias. Tambien veo que importas data.table solo para retornar un objeto de clase data.table sin usar sus capacidades en operaciones. En pocas palabras, ve la forma de reducir los paquetes.

Good point! Thank you! I have erased some packages that could be easily substituted by others, and have moved some that were only used for testing purposes to the suggested packages section

Tambien veo que en documentation escrves importfrom data.table y luego en la funcion, as.data.table, sin data.table::as.data.table. Recuerdo que la segunda forma es la apropiada. Puedes comentar por favor?

The code was initially written with this notation, but another reviewer, bpbond, pointed out in this issue that import from was best practice. That's why I have been following this notation/format accross the package

DESCRIPTION mentiones LICENCE Mit + File but LICENSE file says BSD 2-Clause License. Is that equivalent with MIT?

You are right, the LICENCE didn't match. I re-uploaded the Mit+ LICENCE

As I recall, you need to have a directory named paper in your repo with the paper in it, probably built with rticles.

Sure! The paper directory was in another branch, but I have moved it to the main branch as suggested. I will be easier to find ;)

-Paper:

First phrase is too long (L9-12).

Code in summary is not needed. To that end, use the code Functionality section, where it can be a subsection or so.

What is the reference for gcamextractor?

Figure 2 does not provide much information. A flow diagram would be more helpful for new users. (suggestion)

Thank you for the manuscript suggestions.

The first sentence was re-written
The code example was moved to the Functionality section, where two subsections were created
The gcamextractor was properly cited
Figure 2 was modified to show the user interface screenshot and the user management workflow. Thank you for the suggestion, it will help to understand better the usage workflow

I could not access the help index, is that OK?

I'm sorry, but where did you see this?

Thank you so much, Best, Clàudia

ibarraespinosa commented 5 months ago

Hi, I was able to see the help index. I checked the documentation and functions, and paper, and all seems ok to me

bc3LC / gcamreport

Review for JOSS #6

first example does not work