Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added more R scripts to Scripts library #18

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

derekagorhom
Copy link
Contributor

Fixes IDEMSInternational/R-Instat#9247

@rdstern , @N-thony
I and beryl have run all the scripts and made corrections to import, export and save codes.
This PR is ready for Review.
Thanks

N-thony
N-thony previously approved these changes Nov 22, 2024
@rdstern
Copy link

rdstern commented Nov 23, 2024

@derekagorhom and @N-thony I am really pleased how this is progressing and also that Darek is involved. But Derek, there is an additional dimension I would like from you and also from Antoine in the checking that I haven't seen yet.

So the starting points is the practice document and the script is now a 3rd way of running the same materials - or similar.

One item that is being done is to have improved comments, which is one aspect of a good script. I think that's happening. But I did ask Beryl to also consider the quality of the R code, and I think this was not something she was able to do. So, Derek could you consier and report on this as needed. It should lead to some issues.

a) For example, I looked quickly at Tutorial 1 and found the R code for one variable summarise and one variable graphs was different. Usually, from a dialog, the code breaks into 3 parts, namely:

  1. Getting the data from the R-Instat data book
  2. Running an R command
  3. Doing something with the results from the command

That seems clear from the summarise code. I found the graph code seems to do 1 and 2 together. I would like the code to set a good example, so I suggest we make an issue, and improve the code for the one variable graph dialog. Then, well have to generate the script again of course.

I have no problem if we accept this pull request, but you should comment, in the script this round, that the code is being improved and a new script will later be generated.

So this round of scripts I would also like Antoine to check that you are doing that, and have been commenting senstibly on those dialogs that need improvements.

b) This mean that each script should also towards the top have a date statement. Something like:
# Script prodiced on 23 Nove 2024 using R-Instat Version 0.8.0.

If then there are changes, add another line:

Revised on 12 Dec 2024, with R-Version 0.8.1 changing lines x and y, changed script from One Variable graph dialog.

Thanks

@derekagorhom
Copy link
Contributor Author

derekagorhom commented Nov 25, 2024

@rdstern I have done part b) of your comment which is the date statement and they are one all the five scripts.
for the part a) i will made an issue on the output naming system change for both One variable graph and One variable frequency.

@rdstern
Copy link

rdstern commented Nov 26, 2024

@derekagorhom and @N-thony if I under, stand correctly, then I think this is probably great - except please give more precise details of exactly what you have changed on each dialog. But I may have misunderstood.

Let me be clear.
I assume you are changing the script(s) and I am really happy with this development.

In some dialogs this work will also lead to changes in the R-code that is generated by that dialog. Is this being reported here, or is this separate, and reported in the main R-Instat github site?

If these changes are reported here, then that is the part that we need to check really carefully. If not, then we just need to note that these scripts will change later, once those issues are undertaken.

Thanks

@derekagorhom
Copy link
Contributor Author

derekagorhom commented Nov 26, 2024

@rdstern the majority of the code changes were mostly in relation to where we will import the file from or where we save files from R-instat.
If you check here and here
You can see the issues raised were more changing the code to a more general way so as to prevent errors when the users reach that point since than would not be how their PC directory is looks like.
However, the way R-instat dialogue does them originally is the correct way, so that when the user is in the script window they can see where the R files are saved or loaded from.

I believe when a user compares these two code:

saveRDS(file=file.path(Sys.getenv("USERPROFILE"), "Documents", "computer_skills.RDS"), object=data_book)

and
saveRDS(file="C:/Users/Ainz Ooal Gown/Documents/gh.RDS", object=data_book)
I believe the second one is easier to understand and that is what R-Instat creates, the only problem is that the second code is only applicable to a particular User's PC while the first can be used for most PCs running the Windows OS.

@N-thony can also bring his suggestion on this
Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Continued work on practical R-Instat scripts for our library
3 participants