search

openjournals / joss-reviews

Reviews for the Journal of Open Source Software
Creative Commons Zero v1.0 Universal
708 stars 38 forks source link

[REVIEW]: stacomiR : a common tool for monitoring fish migration #791

Closed whedon closed 5 years ago

whedon commented 6 years ago

Submitting author: @MarionLegrandLogrami (Marion Legrand) Repository: https://github.com/MarionLegrandLogrami/stacomiR Version: 0.5.3.1 Editor: @arfon Reviewers: @steffilazerte, @boshek Archive: 10.5281/zenodo.3371259

Status

status

Status badge code:

HTML: <a href="http://joss.theoj.org/papers/0cacbf19f5822f668e915f788dec1a82"><img src="http://joss.theoj.org/papers/0cacbf19f5822f668e915f788dec1a82/status.svg"></a>
Markdown: [![status](http://joss.theoj.org/papers/0cacbf19f5822f668e915f788dec1a82/status.svg)](http://joss.theoj.org/papers/0cacbf19f5822f668e915f788dec1a82)

Reviewers and authors:

Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)

Reviewer instructions & questions

@steffilazerte, please carry out your review in this issue by updating the checklist below. If you cannot edit the checklist please:

  1. Make sure you're logged in to your GitHub account
  2. Be sure to accept the invite at this URL: https://github.com/openjournals/joss-reviews/invitations

The reviewer guidelines are available here: https://joss.theoj.org/about#reviewer_guidelines. Any questions/concerns please let @arfon know.

Review checklist for @steffilazerte

Conflict of interest

Code of Conduct

General checks

Functionality

Documentation

Software paper

whedon commented 6 years ago

Hello human, I'm @whedon. I'm here to help you with some common editorial tasks. @steffilazerte it looks like you're currently assigned as the reviewer for this paper :tada:.

:star: Important :star:

If you haven't already, you should seriously consider unsubscribing from GitHub notifications for this (https://github.com/openjournals/joss-reviews) repository. As a reviewer, you're probably currently watching this repository which means for GitHub's default behaviour you will receive notifications (emails) for all reviews 😿

To fix this do the following two things:

  1. Set yourself as 'Not watching' https://github.com/openjournals/joss-reviews:

watching

  1. You may also like to change your default settings for this watching repositories in your GitHub profile here: https://github.com/settings/notifications

notifications

For a list of things I can do to help you, just type:

@whedon commands
whedon commented 6 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 6 years ago

--> Check article proof :page_facing_up: <--

MarionLegrandLogrami commented 6 years ago

@whedon generate pdf

whedon commented 6 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 6 years ago

PDF failed to compile for issue #791 with the following error:

/app/vendor/bundle/ruby/2.3.0/bundler/gems/whedon-195e6d124d0f/lib/whedon.rb:80:in check_fields': Paper YAML header is missing expected fields: tags (RuntimeError) from /app/vendor/bundle/ruby/2.3.0/bundler/gems/whedon-195e6d124d0f/lib/whedon.rb:68:ininitialize' from /app/vendor/bundle/ruby/2.3.0/bundler/gems/whedon-195e6d124d0f/lib/whedon/processor.rb:32:in new' from /app/vendor/bundle/ruby/2.3.0/bundler/gems/whedon-195e6d124d0f/lib/whedon/processor.rb:32:inset_paper' from /app/vendor/bundle/ruby/2.3.0/bundler/gems/whedon-195e6d124d0f/bin/whedon:37:in prepare' from /app/vendor/bundle/ruby/2.3.0/gems/thor-0.20.0/lib/thor/command.rb:27:inrun' from /app/vendor/bundle/ruby/2.3.0/gems/thor-0.20.0/lib/thor/invocation.rb:126:in invoke_command' from /app/vendor/bundle/ruby/2.3.0/gems/thor-0.20.0/lib/thor.rb:387:indispatch' from /app/vendor/bundle/ruby/2.3.0/gems/thor-0.20.0/lib/thor/base.rb:466:in start' from /app/vendor/bundle/ruby/2.3.0/bundler/gems/whedon-195e6d124d0f/bin/whedon:99:in<top (required)>' from /app/vendor/bundle/ruby/2.3.0/bin/whedon:22:in load' from /app/vendor/bundle/ruby/2.3.0/bin/whedon:22:in

'

MarionLegrandLogrami commented 6 years ago

@whedon generate pdf

whedon commented 6 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 6 years ago

PDF failed to compile for issue #791 with the following error:

/app/vendor/bundle/ruby/2.3.0/bundler/gems/whedon-195e6d124d0f/lib/whedon.rb:80:in check_fields': Paper YAML header is missing expected fields: tags (RuntimeError) from /app/vendor/bundle/ruby/2.3.0/bundler/gems/whedon-195e6d124d0f/lib/whedon.rb:68:ininitialize' from /app/vendor/bundle/ruby/2.3.0/bundler/gems/whedon-195e6d124d0f/lib/whedon/processor.rb:32:in new' from /app/vendor/bundle/ruby/2.3.0/bundler/gems/whedon-195e6d124d0f/lib/whedon/processor.rb:32:inset_paper' from /app/vendor/bundle/ruby/2.3.0/bundler/gems/whedon-195e6d124d0f/bin/whedon:37:in prepare' from /app/vendor/bundle/ruby/2.3.0/gems/thor-0.20.0/lib/thor/command.rb:27:inrun' from /app/vendor/bundle/ruby/2.3.0/gems/thor-0.20.0/lib/thor/invocation.rb:126:in invoke_command' from /app/vendor/bundle/ruby/2.3.0/gems/thor-0.20.0/lib/thor.rb:387:indispatch' from /app/vendor/bundle/ruby/2.3.0/gems/thor-0.20.0/lib/thor/base.rb:466:in start' from /app/vendor/bundle/ruby/2.3.0/bundler/gems/whedon-195e6d124d0f/bin/whedon:99:in<top (required)>' from /app/vendor/bundle/ruby/2.3.0/bin/whedon:22:in load' from /app/vendor/bundle/ruby/2.3.0/bin/whedon:22:in

'

MarionLegrandLogrami commented 6 years ago

@whedon generate pdf

whedon commented 6 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 6 years ago

--> Check article proof :page_facing_up: <--

MarionLegrandLogrami commented 6 years ago

@whedon generate pdf

whedon commented 6 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 6 years ago

--> Check article proof :page_facing_up: <--

steffilazerte commented 6 years ago

The premis of stacomiR is to provide access to data base for monitoring fish migration in a user-friendly way. This is a great idea! I think it's really important to provide access to large biological data sets, especially in a way that is inclusive and which helps users who aren't familiar with R. The amount of work that has gone into this R package is also substantial and I'm eager to see it in action.

Unfortunately, I ran into several major issues when I tried to install and use stacomiR.

1) Lack of documentation While the functions themselves are well documented, there are no tutorials, vignettes or websites with information for users just getting started. Even the documentation revealed by ?stacomiR-package didn't provide any information for a new user, but merely directed the user to contact the author.

2) Installation Dependencies in the DESCRIPTION don't list minimum versions. This may create problems for some users with older package versions. For example, I ran into a problem with the installation because I have Hmisc v4.0.3, but the package required Hmisc >= v4.1.0

3) Usage Possibly because of both the above, once installed I struggled to use the package.

Specific problems

I followed the instructions in the JOSS paper, but was unable to launch the interface with stacomi(), or with stacomi(gr_interface = FALSE, login_window = FALSE, database_expected = FALSE).

I expected the first command to fail as I don't have the database (and it did) but the second command did nothing at all (no output, no errors), and I was at a loss of how to further proceed.

Following the rest of the example I ran into an error with the calcule() function:

> r_mig_mult <- calcule(r_mig_mult, silent = TRUE)
Loading required package: DBI
Error in postgresqlNewConnection(drv, ...) : 
  RS-DBI driver: (could not connect test@localhost:5432 on dbname "test": could not connect to server: Connection refused
    Is the server running on host "localhost" (127.0.0.1) and accepting
    TCP/IP connections on port 5432?
)
Error in !dbPreExists : invalid argument type

The final lines in the JOSS paper gave me no output (presumably because the above function failed). I also struggled to understand what the functions were supposed to do.

Other issues:

Verdict

In the end, I can't say I've actually reviewed the package, as I wasn't able to get it working. However, dealing with remote databases and interactive interfaces are always tricky. I'm still looking forward to giving it a shot and I'm more than happy to work with the authors to figure out what's going on.

For acceptance in JOSS I definitely need to finish my review (i.e. be able to test the package), and there also needs to be some solid documentation (perhaps a package vignette?) that walks new users through getting set up and addresses common problems or road blocks.

My System Info

 version  R version 3.4.4 (2018-03-15)  
 os       Ubuntu 16.04.4 LTS            
 system   x86_64, linux-gnu           
 ui       RStudio                     
 language en_CA:en                    
 collate  en_CA.UTF-8                 
 tz       America/Winnipeg            
 date     2018-06-28
arfon commented 6 years ago

Thanks for your details review @steffilazerte. @MarionLegrandLogrami - please take a look at @steffilazerte's feedback.

MarionLegrandLogrami commented 6 years ago

Dear ​Steffi LaZerte,

First of all, Cédric Briand and I wanted to warmly thank you for the time you spent to test our package. We found your remarks really useful and some will allow us to improve it​​. I will take your remarks/questions one by one and in the order of your message to try to answer as best as possible.

1. Lack of documentation Indeed, the documentation of the R package does not provide any help on how to use the package itself (link to the database, the installations to be made, etc.). To present in more details the package we wrote a vignette but we failed to compile and integrate it into the package. We did not understand why, but one of your remarks enlightened us on this point (I will come back to this later). Until we can integrate this vignette to our package you can already find it at this address : https://github.com/MarionLegrandLogrami/stacomiR/tree/master/vignettes. In addition, we created a space dedicated to this project on "trac" system. This space has been created for users of the stacomi project. They can find different elements to help them to better use this tool. You can access to this space following this link : http://w3.eptb-vilaine.fr:8080/tracstacomi/wiki. You will need to connect to this space using user (utilisateur) : stacomi and password (mot de passe) : stacomi. To give you some background on the fish data banking in France, you should know that there is no centralized system to organize this collection. The data are collected by various associations whose purpose is to follow fish on the long term (especially diadromous fish). Sometimes this is done by the Amateur Fishing Federations and the Migratory Associations (10 in France that work on diadromous fish) of the watershed concerned, pool the informations of the different Federations and make sure that the protocols are as homogeneous as possible. Sometimes it is the Migratory Associations themselves who collect the data (the most frequent case). Finally, there may be public institutions or research laboratories that monitor stations. To date, there is no structure to homogenize this collection or to organize the valuation of these data. It is on this basis that we decided to develop a R package to enable all data producers, whatever the structure, to improve the banking and the valuation of their data and if wanted to share data. Data ownership obviously remains that of producers and only producers can decide whether or not they want to make their data available and in what context. However, the R package is an excellent tool to show data producers the value of sharing information to compare data across multiple stations, multiple watersheds, and the group of Stacomi users that we created Cédric and I allows them to meet each other, share their package practice and make sharing information easier. Ultimately, we want to develop a package using Shiny and that would go further in the valuation and sharing of information.The stacomi project allows to make available a common tool and Cédric and I provide assistance to users concerning the installation of the tool, the difficulties of use, etc.

2. Installation It's a good point, thank you for highlighting it. On rforge website, we indicated that to work the package needs to be run with r 3.5.1 in particular since some functions of the new version of Hmisc don't work anymore with previous version of R. But, it would be better to indicate the version of dependencies in the description as you propose. I wrote a ticket on the trac to remember this improvement for a new version of the package (ticket number 238).

3. Usage We hope that all clarifications gave in the two first point will help !

4. Specific problems The problem you submitted to us regarding the impossibility of connecting to the "test" database gave us an interesting hint to understand why our vignette did not compile. For some calculations that we do in the package we have created a database named "test" that has been created for all users. Nevertheless, a person who does not have the stacomi installation obviously does not have a test database. We will work on a new solution to no longer need this database. I created a ticket on the trac corresponding to this improvement (ticket number 239). The commande line stacomi() works only for users having a complete installation as you guessed. The command line stacomi(gr_interface = FALSE, login_window = FALSE, database_expected = FALSE) allows you to use the package but only in r command line as the interface need a direct connexion to the database. The reason why the next command didn't work (r_mig_mult <- calcule(r_mig_mult, silent = TRUE)) is because we ask in the calcule method of this function to connect to the "test" database. This is a mistake and it is the reason I created a trac ticket (239) to improve the next version of our package. To work around this problem immediately, is it possible to you to create a localhost with UTF8 encoding database under postgresql named "test" (landlord = postgres) and a test login role with user=test and password=test. This will allow the command line r_mig_mult <- calcule(r_mig_mult, silent = TRUE) to work.

5. Other issues

stacomi_localisation_ac_conv_2018_03_23

We hope that these informations will be useful to you. Do not hesitate to come back to us if there are any questions or if you encounter difficulties. Thank you again for the time invested, which was very helpful to us.

MarionLegrandLogrami commented 6 years ago

@whedon generate pdf

whedon commented 6 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 6 years ago

--> Check article proof :page_facing_up: <--

steffilazerte commented 6 years ago

@MarionLegrandLogrami and coauthors, thanks for all the clarifications. This definitely helps me move forward. I got through one of the examples and I'm very impressed with the quality and complexity of the output, I can see how useful a package this is going to be!

Below I've responded to you and at the bottom are some further comments I have

Responses

  1. Indeed, the documentation of the R package does not provide any help on how to use the package itself (link to the database, the installations to be made, etc.). To present in more details the package we wrote a vignette but we failed to compile and integrate it into the package. We did not understand why, but one of your remarks enlightened us on this point (I will come back to this later). Until we can integrate this vignette to our package you can already find it at this address : https://github.com/MarionLegrandLogrami/stacomiR/tree/master/vignettes.

This vignette is very helpful and should definitely be listed somewhere prominently to help new users. If you still struggle to compile it as part of the R package, I think you can still include static files (for example), or at least put a link to it in the documentation of stacomir-package.

  1. In addition, we created a space dedicated to this project on "trac" system. This space has been created for users of the stacomi project. They can find different elements to help them to better use this tool. You can access to this space following this link : http://w3.eptb-vilaine.fr:8080/tracstacomi/wiki. You will need to connect to this space using user (utilisateur) : stacomi and password (mot de passe) : stacomi.

This is good, but for an open source project, it might be better to have an open tracking system as well. At the very least there should be some information for users as to where they can go to submit issues. Consider using the "BugReports: " tag in the DESCRIPTION (for example see here)

  1. To give you some background on the fish data banking in France, you should know that there is no centralized system to organize this collection. The data are collected by various associations ...

This is really helpful background information that will make it clear to the readers of the JOSS manuscript who the intended audience is. Consider including more of these details in the JOSS summary. In particular, I think the scope of this project (i.e. France/Spain and Europe in general) should be more prominently placed.

  1. On rforge website, we indicated that to work the package needs to be run with r 3.5.1 in particular since some functions of the new version of Hmisc don't work anymore with previous version of R. But, it would be better to indicate the version of dependencies in the description as you propose. I wrote a ticket on the trac to remember this improvement for a new version of the package (ticket number 238).

Changing the version of R for the whole package as opposed to the individual package versions is a bit of a brute force approach. Often, people working for institutions or government are limited to older versions of R by their IT departments (definitely a problem in Canada!) so it's nice to keep the R version a package requires as low as possible. I don't think this really matters for the JOSS review, but something to bear in mind for the future.

  1. The problem you submitted to us regarding the impossibility of connecting to the "test" database gave us an interesting hint to understand why our vignette did not compile. For some calculations that we do in the package we have created a database named "test" that has been created for all users. Nevertheless, a person who does not have the stacomi installation obviously does not have a test database. We will work on a new solution to no longer need this database. I created a ticket on the trac corresponding to this improvement (ticket number 239). The commande line stacomi() works only for users having a complete installation as you guessed. The command line stacomi(gr_interface = FALSE, login_window = FALSE, database_expected = FALSE) allows you to use the package but only in r command line as the interface need a direct connexion to the database. The reason why the next command didn't work (r_mig_mult <- calcule(r_mig_mult, silent = TRUE)) is because we ask in the calcule method of this function to connect to the "test" database. This is a mistake and it is the reason I created a trac ticket (239) to improve the next version of our package. To work around this problem immediately, is it possible to you to create a localhost with UTF8 encoding database under postgresql named "test" (landlord = postgres) and a test login role with user=test and password=test. This will allow the command line r_mig_mult <- calcule(r_mig_mult, silent = TRUE) to work.

I'm glad this is on track to get fixed, I think this should be fixed before we finalize things here. Unfortunately, I haven't had time to test to see if the creation of a local data base fixes the problems, and if you plan to fix this anyway, I won't bother.

  1. Authorship

Thank you for the clarification. I was sure there was a rational, but part of my job as a reviewer is checking up on this. @arfon, do you think it could be useful to have authors declare authorship contributions as part of the review application? It's not very common, but I know of at least some journals which require this.

  1. JOSS policies: Does this mean that you recommend to delete the different lines of R code we write on the paper to give some illustration on the possibilities of the package? We saw for example this paper published on JOSS which incorporated command lines of their package R and we thus thought that it was allowed. But if you think it would be better to delete them to give a better overview of what the package can do we are not opposed.

Hmm, that is a good point. I'm okay if you want to keep that example in then, as long as it works (i.e. fixing the test database issue). However, I really think that you need to be clear about what will and won't work for users without a database connection if you're going to include the code. It might be simpler to include a description of the scope of your package, what it does, and then the output of the code (i.e. the image of the user interface, some of the summary figures), installation instructions, and links to the vignette.

  1. Region included: at the moment the database is used all over France (thanks in large part to the banking work I have done for my thesis) and also in Spain. We have planned that the package can be used quite widely and that is why we have set up a system for translating the messages sent by the package in French and English for all messages and in Spanish for only a part (translation system realized with po.edit). To go further and promote the package in other countries, we should be able to put more resources on the translation, but if one day it becomes necessary, everything is planned so that we can do it.

I think this is important information to add to the JOSS manuscript and to the vignette. You don't have to say that it's ONLY for France and Spain, but it's important to let potential users know what the current scope is.

Further comments

  1. If the users need the data base to really use this package (beyond demonstrations), I think this should be made very clear in the JOSS manscript, the vignette, and in the stacomiR-package documentation. For example, it is in the vignette, but I think the information should come before the 'Usage' section, under its own heading ('Database', for example). You should also list the emails of the authors right there so it's easy for users to get started.

  2. As this review is meant for the CRAN version of the software, I think we need to wait for the CRAN version to be updated (i.e. to update the package/R version dependencies and other bugs) before the JOSS review can be finalized (@arfon, is this the case, or is it fine as long as the changes are made to the developmental version?)

  3. I can see that you have quite a few tests in stacomiR > inst > tests > testthat. However, I think to be tested properly, they should be in stacomiR > tests > testhat (see here). You can use skip_on_cran() for any tests that won't work remotely.

  4. JOSS requires that the software have a clear set of community guidelines for 1) Contributing to the software 2) Reporting issues or problems with the software 3) Seeking support

    You can generally include this as a CONTRIBUTING.md file (for example), but also including a link to Bug Reports or tracking in the DESCRIPTION file will also help.

  5. Finally, although we're definitely making progress, I still feel as if I haven't been able to fully test the package because I don't have access to the database. What about providing a demo data base as part of the R package? Alternatively, could you arrange for me to have access for testing purposes?

Thank you for your patience, I look forward to working more with stacomiR!

arfon commented 6 years ago

Thanks for the detailed review @steffilazerte. Responding to a couple of your comments here:

This is good, but for an open source project, it might be better to have an open tracking system as well. At the very least there should be some information for users as to where they can go to submit issues. Consider using the "BugReports: " tag in the DESCRIPTION (for example see here)

This is actually a requirement for JOSS submissions (see https://joss.readthedocs.io/en/latest/submitting.html#submission-requirements):

...the software associated with your submission must:

  • Have an issue tracker that is readable without registration

Thank you for the clarification. I was sure there was a rational, but part of my job as a reviewer is checking up on this. @arfon, do you think it could be useful to have authors declare authorship contributions as part of the review application? It's not very common, but I know of at least some journals which require this.

We're considering this as an editorial board but at this point we don't have a policy for this.

arfon commented 6 years ago

Hi @MarionLegrandLogrami - when do you think you will be able to incorporate @steffilazerte's feedback?

MarionLegrandLogrami commented 6 years ago

Hi @arfon and @steffilazerte - thanks for your message. A very short message just to tell you that we are incorporating the @steffilazerte's remarks. it's hard to tell you when we're done but we've made good progress. I will send you a message as soon as the package is ready and in accordance with the requests made

MarionLegrandLogrami commented 5 years ago

@whedon generate pdf

whedon commented 5 years ago 
Attempting PDF compilation. Reticulating splines etc...
whedon commented 5 years ago

:point_right: Check article proof :page_facing_up: :point_left:

MarionLegrandLogrami commented 5 years ago

Hi @arfon and @steffilazerte - I'm happy to inform you that we finished to update the stacomiR package. The new version is available on CRAN and rforge (we updated the version provided on github as well). We made the main following modifications:

We hope that this new version will meet all your requirements. Again we wanted to thank @steffilazerte for all the really helpful comments that helped us to improve our package.

arfon commented 5 years ago

Thanks @MarionLegrandLogrami! @steffilazerte - please take another look when you get a chance.

steffilazerte commented 5 years ago

I will definitely take a look, but it might be a week or so before I can really dive in fully. I'll keep you posted.

steffilazerte commented 5 years ago

@MarionLegrandLogrami and coauthors,

I'm happy to say that with the updates to the stacomiR package I've been able to run through the examples based on the built-in example data sets!

The vignette is a wealth of information and a valuable addition. By running through the example data, the plot/report documentation and checking back with the vignette's section on Data structure, I was able to gain a much better understanding of the kind of data being summarized (which is impressive, given how little I know about fish monitoring).

I was able to snoop through most of the documentation and create many different graphical summaries of the included data sets. I explored figures and summaries for these data sets and, once again, I am seriously impressed by the variety of analyses included.

Including instructions for installing/restoring the database on GitHub is a great idea, and I like that you've linked to them in the JOSS manuscript as well.

The downside is that I am still unable to setup the database, and I haven't been able to launch the user interface.

I've also realized that your setup is designed for Windows (whereas I am on Linux). For example, I will never be able to have the location c:\users\my_session\Documents\CalcmigData on my computer, as Linux file paths do not have drive locations (c:\) (and neither do Macs).

Below are the error messages I got when attempting to restore the database. I suspect that the locale error is related to the Operating system differences, but I am not sure if the database "bd_contmig_nat" does not exist error is the result of the locale error or something else.

postgres@steffi-desktop:~$ psql postgres < install_bd_contmig_nat.sql 
SET
SET
SET
SET
SET
 set_config 
------------

(1 row)

SET
SET
SET
ERROR:  invalid locale name: "French_France.1252"
ERROR:  database "bd_contmig_nat" does not exist
\connect: FATAL:  database "bd_contmig_nat" does not exist
postgres@steffi-desktop:~$

Suggestions

Conclusions

@arfon, I have evaluated everything I can here. The package authors have addressed my comments and concerns and I have run through all the JOSS review check boxes, with the exception of fully testing the functionality (i.e. accessing the database in order to create reports of novel data subsets and the graphical user interface). Admittedly, these aspects are a pretty big part of this R package.

I would suggest either accepting the revisions as sufficient, or finding another, windows-based reviewer to finish the review. My apologies for not being able to take this to the end. I hope this hasn't been too inconvenient for everyone involved!

arfon commented 5 years ago

@arfon, I have evaluated everything I can here. The package authors have addressed my comments and concerns and I have run through all the JOSS review check boxes, with the exception of fully testing the functionality (i.e. accessing the database in order to create reports of novel data subsets and the graphical user interface). Admittedly, these aspects are a pretty big part of this R package.

Thanks for your excellent review @steffilazerte and your suggestion here. I think we should seek an additional reviewer to help verify these aspects of the package.

@MarionLegrandLogrami - do you know of anyone who might be able to help with this? @karthik - do you know of any Windows/R users that might be able to help?

ooo[bot] commented 5 years ago

:wave: Hey @arfon...

Letting you know, @karthik is currently OOO until Saturday, December 1st 2018. :heart:

karthik commented 5 years ago

/ooo away until tomorrow

karthik commented 5 years ago

@arfon Sure, let me come up with some ideas

(apologies about ooo. Trying to figure out how to unset the test date).

MarionLegrandLogrami commented 5 years ago

@steffilazerte, my colleagues and I wanted to thank you once again for all the work done and the help provided. Thanks to you we made lot of progress ! For the error concerning the database

database "bd_contmig_nat" does not exist I think this is not related to the operating system. I saved the database using the pg_dump command with the -C option to insert in the dump file the command line needed to create the database but unfortunately (I do not understand why and I did not see this problem before reading your message) the psql command fails to restore the database without the user manually creating the database using the following SQL command line: CREATE DATABASE bd_contmig_nat WITH ENCODING = 'UTF8'; I will add this command line in the github instruction file for the installation of the database.

Thanks again for all your work and all your remarks and hints.

steffilazerte commented 5 years ago

Oh that's useful, I'm glad I was able to help at least a little! Good luck!

MarionLegrandLogrami commented 5 years ago

@karthik Hello, thank you for your help in trying to find an additionnal reviewer. Do you have some suggestions ? @arfon I'm sorry but I don't know who else to propose to help finish the job

ooo[bot] commented 5 years ago

:wave: Hey @MarionLegrandLogrami...

Letting you know, @karthik is currently OOO until Saturday, December 1st 2018. :heart:

arfon commented 5 years ago

Friendly reminder to give us some suggestions here @karthik if you have any?

karthik commented 5 years ago

I'd recommend @boshek if he's got time.

arfon commented 5 years ago

:wave: @boshek - do you think you might be able to help us here? We're very close to finishing this review but unfortunately @steffilazerte wasn't able to verify some of the functionality of the software.

boshek commented 5 years ago

Hi @arfon - Here is what input I have at this time. Hopefully this is helpful to the process here. Much of my experience mirrors that of @steffilazerte.

This paragraph seems to contain some information that likely should be shared as code. That is, how do you do this?

You also need to add new connection role: In pgAdmin, you need to create a connexion role with the same name as your schema (for example if I write inside schema called 'toto' I need to create a connexion role with name 'toto' and password 'toto' (or wathever password you choose)). Finally, you need to update GRANT in the different table of your database to allow java or r programm to connect to your data. to do so, download the Updating_grant file (in the installation folder), unzipp the file and open the sql script in your pgAdmin. Using search/replace update all the user_1 in the script by the name you choose for your schema. When you have made all the modifications select all the script and execute (Ctrl+e).

I'm obviously missing something as I'm not clear on what to do with the .sql file. 

> updating_grants <- readLines("Installation/Updating_grant.sql")
> 
> any(grepl("toto", updating_grants))
[1] FALSE

I got this error trying to call the ui:

> stacomi()
Error in funout(msg3, arret = TRUE) : connection impossible
In addition: Warning messages:
1: In RODBC::odbcDriverConnect("DSN=bd_contmig_nat;UID=iav;PWD=iav",  :
  [RODBC] ERROR: state IM002, code 0, message [Microsoft][ODBC Driver Manager] Data source name not found and no default driver specified
2: In RODBC::odbcDriverConnect("DSN=bd_contmig_nat;UID=iav;PWD=iav",  :
  ODBC connection failed

I really am quite interested to see the interface. I've never quite seen R used this way.

Outside of testing on Windows which so far I have unable to accomplish I'd suggest two steps that would make this package more usable:

could likely be turned into a function using some combination of download.file and unzip. Another example of this is the sql used to create the database. I believe some of this can directly be run from R (as well as the above download/unzip workflow described above). Despite that my (limited) attempt ended like this:

> DBI::dbExecute(con, readLines("Installation/install_bd_contmig_nat.sql"))
Error in result_create(conn@ptr, statement) : 
  Expecting a single string value: [type=character; extent=1241928].
filecsv<-"C:/Program Files/stacomi/calcmig.csv"

with this: 

> filecsv <- file.path(rappdirs::site_data_dir("stacomiR"), "calcmig.csv")
> filecsv
[1] "C:\\PROGRA~3\\stacomiR\\stacomiR/calcmig.csv"

There may be other instances that I haven't found that rely on hard-coded paths specified in Windows. Somewhat related, a PostgreSQL database on Windows is slower than one on Linux so I think there would be an even higher demand for this outside of Windows.

MarionLegrandLogrami commented 5 years ago

Hi @boshek . Thanks a lot for your comments. First of all, I wanted to tell you that we are looking at your proposal to simplify the installation process, but we need some time to do it. I will come back to you when it's over Regarding the use of multiple platforms, the stacomiR package is already working on linux. We have not checked the use of the package under Linux because we are all working on Windows, but we will consider configuring a virtual machine with Linux to test it. At this moment, all the users of this package are using windows operating system, except one that is using OS Mac. All these users are not very familiar with R (most have never used R) no more than with databases and ODBC link creation. For all these users we made all the installation for them. But we agree that trying to find a way to simplify this installation would be a good thing.

To respond more specifically to some of your comments :

You also need to add new connection role: [...]

You need to do this only if you want to use the database (= add data) and you want to change the name of the user's schema (I will try to explain that better on github). To simply test the package you don't need to do that as you have the IAV schema with all data to test the package.

It's the same for the Updating_grant.zip file. You must run the file only if you want to change the schema username (you just need to open the SQL script file in pgAdmin and execute the code).

Regarding the error you get:

stacomi() Error in funout(msg3, arret = TRUE) : connection impossible In addition: Warning messages: 1: In RODBC::odbcDriverConnect("DSN=bd_contmig_nat;UID=iav;PWD=iav", : [RODBC] ERROR: state IM002, code 0, message [Microsoft][ODBC Driver Manager] Data source name not found and no default driver specified 2: In RODBC::odbcDriverConnect("DSN=bd_contmig_nat;UID=iav;PWD=iav", : ODBC connection failed

I'll be back to you shortly to inform you of our progress on the improvements you have submitted to us.

labarba commented 5 years ago

👋 @MarionLegrandLogrami — We haven't heard from you in a while. What's your status? Are you planning on improving the software in view of the feedback you have received here?

MarionLegrandLogrami commented 5 years ago

@labarba - thank you for your message. We are still working on the change requested by our reviewer. We are almost done (still a few checks) and try to finish as soon as possible.

MarionLegrandLogrami commented 5 years ago

Hi @boshek and @labarba - We are happy to inform you that we completed the development of the automated installation of our package stacomiR and that we commited the scripts on github. We produced two versions of the automated installation: one for Windows and the other for Linux. To date, we thoroughly tested the installation under Windows. We still have some checks to perform on the Linux version. Indeed, we checked that the script creates the database, restores the data available on github and creates an ODBC link but we still have to test the launch of the package (in particular, check that the connection is well established with the database). We plan to do these final checks (for the Linux version) by the end of next week. We hope that this development will meet your expectations. We believe that it will significantly simplify the installation of our package and make it more easily accessible. We wanted to thank @boshek for this hint.

MarionLegrandLogrami commented 5 years ago

Hi @boshek - a little word just to let you know that we commited on github the final version of the automated installation for Linux. I forgot to tell you last time that we first tried to developp an R script to make all the installation (we tested the functions you suggested), but we failed. It's the reason we thought of an other way with batch files. We hope this will meet your expectations.

arfon commented 5 years ago

:wave: @boshek - could you please review the changes that have been made by @MarionLegrandLogrami and let us know if these satisfy your concerns?