Moving instat code from R-Instat into separate R packages

[lilyclements]

I was too anxious to wait, so I tried it out for the instatCalculations.R file, and it worked for the testing I did.

The method:

• Removed instat_calculation and calculation R6 files
• Removed all instances of instat_calculation$set, and calculation$set
• Replaced all instances of instat_calculation$new with instatCalculations::instat_calculation$new
• Replaced all instances of calculation$new with instatCalculations::calculation$new (and then if we had instatCalculations::instat_instatCalculations::calculation$new, replaced with instatCalculations::instat_calculation$new)

To-do:

1. Where do we add in the devtools::install_github("IDEMSInternational/instatCalculations") line so that we can be sure that it is correctly loaded into R-Instat (presumably in Rsetup.R).
2. Where do we add in the library(instatCalculations) R code (presumably in Rsetup.R)
3. Currently I have set it up that it runs as the function is instatCalculations::instat_calculation$new in our dlg R code. We want to separate this out to be

XXX.SetPackageName(instatCalculations)
XXX.SetRCommand(instat_calculation$new)

This is just a draft/test illustration of the approach to do this, and a test to see if it works!

[lilyclements]

@rdstern if you wanted to view or test, then this is the PR

[lilyclements]

E.g., here we want to separate into

clsJumpCalcFunction.SetPackageName("instatCalculations")
clsJumpCalcFunction.SetRCommand("instat_calculation$new")

I just did it this way as a quick check to see if it would actually work. I'm very surprised it worked for the testing I did.

[rdstern]

@lilyclements very exciting. I was earleir assuming we should make a parallel version, while we make these changes. But sorting it out package by package, instead, would be great.

I tried, with the current system but can't open a dataset?

I have a problem on my test version of not having all the packages we need - and am gradually adding them. But I think this may be different.

[rdstern]

@lilyclements I tried again, first adding all the packages into my test version.
I still get the same error, so I think it is at your end. And excited that you said you thought you might know where it is.
I assume to test, I should use the calculator and maybe also the summarise dialog etc?

[lilyclements]

@rdstern great - can you try again now? I cannot replicate it my end, but I could find the line where that error was occurring (and other places where it would occur in the future), and it should hopefully be fixed now :)

[rdstern]

@lilyclements very exciting. I report on my tests so far:
a) I had lots of trouble installing instatCalculations in the test version. I tried many options and nothing seemed to work, including the Import saying the directory didn't exist. However, I suspect this is a glitch on my version only, so report just in case others have difficulty. However, I stopped and started again (maybe should have done that earlier) and it now said it was installed and up-to-date. Whew. So I started testing.
b) Now I could import from the library - examples of datasets came in fine. They hadn't before and seemed to need the package, because they complained when it wasn't installed. Incidenetally I suggest that it and databook be included in the run, so the package name is not needed when used.
c) I tried importing a dataset from a file, and also a new data frame. All worked fine. Those are the top 3 dialogs in the File menu.
d) I tried a few calculations - R-Instat calculator and that was ok.
e) I tried the Prepare > Column: Numeric > rounding data, but on a selection of variables from diamonds - so using select as well. All ok.
f) I then tried the Prepare > Reshape > Column Summarise and got my first error. I tried simple summaries on our usual small survey data:

g) I assumed that if summarise doesn't work, then the Describe > One Variable > Summarise > Customised would fail, but it is fine!

h) So I went back to the Prepare > Reshape > Summarise dialog. I found it works fine with the new features, so when there are no factors. And that was added quite recently. I wonder if that added something into the code that is not in the version of instatCalculations that you have now?

All very promising

[Patowhiz]

@lilyclements good to see that the calculation code is being separated out. I have many plans that relates to iterating from the original code. Are you looking towards releasing it with documentation or that's a low priority at the moment?

[rdstern]

@Patowhiz glad you like it. If we mean the same on documentation, then that's done already - and for all the R-Instat packages, and also available and linked from the help. So, once working the documentation will also - like the other packages - be available in your maximised window, so without needing the internet links.

In addition, these packages are not yet in CRAN, but are in IDEMS github site, and the import packages dialog has been amended so they can easily be installed.

This is another stage in this huge step forward!

[lilyclements]

@rdstern you're absolutely right that I hadn't updated instatCalculations since we added in param_list. It's now updated, so if you update your instatCalculations file, this should now work!

(I did update databook relatively recently, but, I imagine that will need re-updating since that is such a large package!)

[lilyclements]

@rdstern I have updated databook now (all the undo functions and what not were not in there). I will need to update them again in the future from R-Instat, but, they're up to date as of the Master version today (I've noted down more specifics in the issues on their repos so I can be more precise on where to update from -- e.g., the most recent commit, etc)

[rdstern]

@lilyclements you said it would be easy to resolve conflicts?

[lilyclements]

@rdstern thanks for this, yes I've fixed it now

[rdstern]

@lilyclements so I updated instatCalculations and still get the same unused argument error as shown above?

[lilyclements]

@rdstern interesting. I'm not getting this problem. Can you update the branch, and can you double check that instatCalculations has downloaded successfully for you by seeing the status on the "Download R Packages" dialog (see below)

Then just try it again in case updating the branch or reopening R-Instat after downloading the R package helped the issue.

If it is still occurring can you share with me your R script so I can investigate?

[rdstern]

@lilyclements I think you have located the problem:
a) In the test version - using your branch - I updated the instatCalculations package. It seems to update fine and here is the code in the output Window:

However, when I return to the importing dialog, the Check remains Green!

I have tried different options, like the upgrade = never, but it always remains green!

I then tried with Version 8.1 and it goes green, as above, after installation!

Aha, I still don't kn ow why it won't work in R-Instat for me, but I tried via RStudio, and now it is brown in R-Instat. Phew. Let's see now!

Now I restarted your branch, it is still brown and no error!! Yippee.

@N-thony really good if you could check now anyway - even if we don't merge yet. No danger, because I think you can't merge a branch that is labelled draft.

[lilyclements]

@rdstern very interesting! This is something we should investigate a bit further for installing packages in the future. I'm not sure where the R-Instat packages store to when installed, and if this is the same or a different location to that of when we do it in RStudio. Glad that you could test it at least and it's working for you now!

[rdstern]

@lilyclements Version 0.8.2 is now released. So we are ready for you to make this big change. I suggest you change the title of this pull request and then include the remaining packages first? I assume the functions within the current code will now also be deleted at the same time. Can we (I mean you?) do this and merge this week?

Once merged, then I assume - if all goes well - all the other open pull requests need to update, so their branch uses the latest merged version. Then they should be ok again?

[rdstern]

I approved, because it seems to work well. I was originally thinking we could merge a package at a time. But I now feel that the merging could induce conflicts in the other open pull requests. @N-thony is this correct? If so it will be better to add the other packages and then do a once and forever merge. With this assumption I'm withdrawing the approval for just the one package.

[rdstern]

Include the reamining packages

[rdstern]

@lilyclements running that gives:

function (output_object_list, object_type_label = NULL, excluded_items = c(), 
    as_list = FALSE, list_label = NULL) 
{
    if (is.null(object_type_label)) {
        out <- names(output_object_list)
    }
    else {
        out <- names(output_object_list)[sapply(output_object_list, 
            function(x) any(identical(x$object_type_label, object_type_label)))]
    }
    if (length(out) == 0) {
        return(out)
    }
    if (length(excluded_items) > 0) {
        excluded_indices <- which(out %in% excluded_items)
        if (length(excluded_indices) > 0) {
            out <- out[-excluded_indices]
        }
    }
    if (as_list) {
        lst <- list()
        if (!is.null(list_label)) {
            lst[[list_label]] <- out
        }
        else {
            lst <- as.list(out)
        }
        return(lst)
    }
    else {
        return(out)
    }
}
<bytecode: 0x0000012b97d06300>
<environment: namespace:databook>

I have also tried adding library(databook) but still get the error.

[lilyclements]

@rdstern - great. This says to me that it is reading in that function correctly. Now, I just need to understand why for you it is saying that databook cannot find it, and for me it is not saying that.

Are you still getting the issue with Excel after updating your branch?

[rdstern]

@lilyclements That's interesting. If I understand correctly, then these aren both working correctly on your machine?
My first theory then is that it might have something to do with the R packages not installed on my machine. Let me try one thing then next.

[lilyclements]

Interesting. Yes, they're both working on my machine.

[rdstern]

@lilyclements just to be clear. I can import quite some rds files, but not this one:

eastern_tidy_with_summaries_2.zip

which imports with version 0.8.2 but not with this.

And can you check with a csv file. I could not get any of them to show a preview and hence Ok wasn't enabled.

With the Excel files it gave that nasty message, but Ok was enabled and it imported fine - one sheet or multiple sheets.

[lilyclements]

This is great - we're on the same page! I'm getting the same errors - I will look tomorrow at

1. Importing this RDS file you shared (issue is occurring from it having calculation stuff in there - and I've missed a link to our instatCalculations)
2. Getting the viewer up for excel files (I didn't even clock it wasn't there, I was focussing too hard on the OK being enabled)
3. Getting CSV files to work again
4. Checking the code for other formats

[rdstern]

@lilyclements thanks for letting me know. That's a big relief - it is all so much harder if the problems are only on some machines!

So I am testing a bit more to see if that helps.
a) I opened a complicated rds file from the Instat library in Climatic > Ghana with summaries - so similar to the Zambia file that I sent, but just 2 stations. That opens fine. I saved it as rds and opened again, no problem. I exported all datasets to xlsx format which works. Then opened the Excel file in R-Instat again. It opened the same as other Excel files - it is just the message and the preview that doesn't work
b) I have the same problem opening the tricot climatic data (rds file) as the zambia rds file I sent you?
c) So a common feature is that these are recently made rds files. I wondered if I open the Zambia file in an old R-Instat and save it, then it is ok. No. I can open the Zambia file in R-Instat Version 0.7.16 and save from there. Then I still get the same error.
d) I have rampaged through quite a lot now, using data that I can import. Stack, summarise, climatic stuff, etc. It is all working so far. I expected more oddities, but everything else I am trying is working! So very well done - wonderful. Youve done brilliantly. That makes these importing problems more puzzling. Why just that? And with the rds files, why just those?
e) Spent the last hour on work using R-Instat, where I use this version as my new one. All still fine. Very odd that the problems are there and only there.

[lilyclements]

@rdstern after a good few hours of hunting, I think I've found out what the issue is, and I don't like it.

Since our .rds files were saved with an old version of DataBook (when it was still tied to R-Instat), it is trying to find old method references when we load them in - references to now non-existent code in R-Instat, not code from the new databook package. This definitely needs to involve a conversation with David to help iron out and work out the best approach going forward.

For now, some things need to remain in the environment to avoid this error and help in testing. Only very few bits, and if you do get another error then let me know and I can amend as needed.

I'll discuss with David how we can sort this out going forward. Perhaps when importing if there's an error we can detect if it's because it's with a RDS file attached to the R-Instat code instead of the databook code, and offer to update and resave it accordingly. I am not sure how this works on the code side, but is certainly something i need to discuss with David regardless on the very least of how we can conceptually handle this.

For the RDS files I was having issues with, it is now working with this temporary solution. This is a great bug to find, but definitely not one I was expecting at all.

If you update your branch, the RDS files should work now!

@rdstern are CSV and XLSX working for you now? They are for me again, so I hope they are for you too. I did do an update to databook.

[rdstern]

@lilyclements delighted you found that, and that's a good logical reason for that problem. Interesting that the olde rds files, e.g. Ghana climatic are ok, and the newer ones, e.g. Zambia, are not.

And the good news remains that I have now tried a lot of tother things, and they seem to work.

And a question also on the other packages in R-Instat. Where do they fit into the new "system". This is more for me to understand the current situation, rather than more urgent work.

1. Danny's naflex package. This is a CRAN package. It is installed in the current version, 0.8.2. Do we use the package in those functions from the naflex package, or are they as functions in the instatCalculations package? And what happens in the new "system". I note it isn't loaded for me in the test version, so I deduce we may not be using the package, but (presumably) the instatCalculations package is using the naflex functions?
2. Lily's openappr package. This is a CRAN package. It is not installed yet in either version 0.8.2 or in my test version? Where does it link - do we need a new dialog?
3. Lily's rapidpror package. This is in github and is installed in Version 0.8.2 and in my test version. There is a File > Import from Rapidpro dialog. I assume it is used there. Anywhere else? Note that the help on this dialog is absent, and the help button on the dialog doesn't link yet. The help context is 10 and needs to be added.
4. Lily's epicsawrap package. This is in github and installed in Version 0.8.2. I have installed it into my test version. I am assuming we already use this package. It links naturally to a climatic dialog and will be expanded with forthcoming epicsa work in 2025. Note the Export to Google Buckets dialog help doesn't link yet. The help context needs to be added. It is 692.
5. Lily's carbonr package. This seems to be in CRAN but is not yet installed in Version 0.8.2. Where does it fit? Do we need a dialog. It is in the Instat packages drop-down. Do we want to keep a copy of the CRAN packages there as well, so we could install the github version of the package if needed?
6. climdex_pcic package. This was in CRAN. Then Lily moved it into our github directory. Now the functions have been moved into the instatclimaticr package. We may still want to provide access to the original CRAN documentation for these functions. I wonder how we can do this?
7. Lily's rpicsa package. This is in the drop-down list and is in github. It is installed in Version 0.8.2. It is not mentioned in the current help file and I don't know if there is a dialog to link to it?
8. Lily's cdms.products package. This is in the drop-down list and is in github. It is available, but not nstalled in Version 0.8.2. It is not mentioned in the current help file and I don't know if there is a dialog to link to it?
9. Lily's networksGraphsR package. This is in the drop-down list and is in github. It is available, but not nstalled in Version 0.8.2. It is not mentioned in the current help file and I don't know if there is a dialog to link to it?

[rdstern]

@lilyclements I have just been using your branch, and have yet to update, because of your Friday work. But, but, but I was importing some Excel data and look at this:

It is perfect! Why is it suddenly working. I was assuming I would blindly press Ok, because there is no preview. Now, lo and behold it is there? Let me continue. I'm combining testing just now, with some Zimbabwe work! But this is very odd.

By the way, up above I also have a question on the different R-Instat packages.

[rdstern]

@lilyclements I don't understand anymore! Here is your branch importing a new Excel file. And it looks perfect! I noticed this yesterday when I needed to upload 4 sheets. I thought I could read them without having the preview. Now, lo and behold, here is the preview!

This is before I update the packages. I wonder which of the 4 you have changed and whether I can update from R-Instat, or need to go to RStudio?

And here is the first csv I have come to:

And the rds files that gave the error, now import fine too. What is going on?

[N-thony]

@lilyclements just a few concerns I have

1. What will happen if we have an issue with one of the R functions that need to be updated or bug fix?
2. I see you have removed the code in the instat_object and data_object files. Won't it be nicer instead to archive the files?

[lilyclements]

@N-thony happy to archive instead of remove, provided they don't run on upload and save to the environment. I have been removing in the time being to ensure it is definitely using our package, and not the VB code.

To update or fix R code, we will now go through the packages. This is actually simpler in debugging than we've had before now (in my opinion..). Roger and I have agreed we will need to arrange a meeting with everyone to go through this new system - and I'll create some resources too that explains in (resources = word document?). What else would be helpful?

[lilyclements]

@rdstern I'm glad that's all coming in fine now - I hope that is because of some changes I made on the branch. I spoke briefly with David on the RDS file, and he seems to agree with me on how we can fix it, but we definitely need another conversation on it (an actual meeting!)

[ChrisMarsh82]

@lilyclements just a few concerns I have

1. What will happen if we have an issue with one of the R functions that need to be updated or bug fix?
2. I see you have removed the code in the instat_object and data_object files. Won't it be nicer instead to archive the files?

I don't see what you mean by archive? If the code is not used delete it. Git will always allow you to go back to an older version if needed.

@lilyclements Regarding what we do if there is an issue. Do you have any unit tests on the functions. This could help with future updates as we make sure we are not breaking existing functionality

[ChrisMarsh82]

@N-thony Can you update install_packages and InstallPackages and link the PR to this one.

[rdstern]

@N-thony and @berylwaswa and @lilyclements I suggest a plan also to help with your justified concerns, Antoine.

I was just writing our cautious plan, but now have an alternative, see below:
a) Lily is away in a meeting all this week. I suggest I will continue to test her branch and it would be good if you and Beryl could do the same. Antoine I suggest you are more an installer of her branch and then, initially, a tester here, rather than a code checker. That can come later.
b) To test, you just go to the branch as any other, but then you need to include the 4 main R-packages first. I hope this will just use the Tools > Install R Package, and the second button at then top.

There are 4 packages, not yet in the drop-down list - so type the names:
instatCalculations, instatExtras, instatClimatic and databook

Once installed, as shown below, the check goes brown:

I hope they install easily for you. For me some stayed green, because they didn't install properly. The errors were sometimes shown in Visual Studio. Otherwise I installed from RStudio.

An alternative is to produce a trial version. Not for the website, but to make the checking easier to do? I suggest updating the packages is easier in a "proper" version. I have the impression that @N-thony can now produce a trial version reasonably quickly. Updates to the packages can then be added.

This trial version could be useful for another reason. We have just merged the outfilling dialog, and this is needed next week by @jkmusyoka . If it is working ok, then he could use this version in Zambia?

[N-thony]

OK, @rdstern, now can you try running the following script:

remove.packages(c("instatExtras", "instatClimatic", "databook", "instatCalculations"))

@lilyclements, I'm updating the installation scripts for packages. Do we always need this line?

[N-thony]

@lilyclements just a few concerns I have

1. What will happen if we have an issue with one of the R functions that need to be updated or bug fix?
2. I see you have removed the code in the instat_object and data_object files. Won't it be nicer instead to archive the files?

I don't see what you mean by archive? If the code is not used delete it. Git will always allow you to go back to an older version if needed.

I think it is a bit more clear to me after @rdstern's comment below.

[N-thony]

@rdstern I have been testing this, and so far it works fine on my end. When all ready, we can get this merged and create a test installer.

[lilyclements]

@lilyclements, I'm updating the installation scripts for packages. Do we always need this line?

@N-thony no we don't need this line. We can also remove the line data_book <- DataBook$new() because that gets read in in the environment when we call in the databook package now

[rdstern]

@lilyclements I am now suggesting we merge this tomorrow and also produce a new version of R-Instat. This could be 0.8.3, but is mainly designed for testing and also to be used by @jkmusyoka in Zambia next week. He needs the new outfilling dialog that was merged yesterday.

@N-thony if you are happy to do this, then I suggest the following strategy:

a) Could you check the pull requests I have approved and merge what you can, They are:
#9454 , #9463, #9458 , #9456 , #9446 , #9452 and #9319 (if @ChrisMarsh82 approves).

b) Then merge this one. And produce the next version.

c) Then @jkmusyoka , @berylwaswa and I will test this new version until the end of the week. The developers continue with their work, but no other pull requests are merged until next week.

How does that sound?

[N-thony]

@lilyclements I am now suggesting we merge this tomorrow and also produce a new version of R-Instat. This could be 0.8.3, but is mainly designed for testing and also to be used by @jkmusyoka in Zambia next week. He needs the new outfilling dialog that was merged yesterday.

@N-thony if you are happy to do this, then I suggest the following strategy:

a) Could you check the pull requests I have approved and merge what you can, They are: #9454 , #9463, #9458 , #9456 , #9446 , #9452 and #9319 (if @ChrisMarsh82 approves).

b) Then merge this one. And produce the next version.

c) Then @jkmusyoka , @berylwaswa and I will test this new version until the end of the week. The developers continue with their work, but no other pull requests are merged until next week.

How does that sound?

@rdstern I'm okay with this plan. But also, this PR depends on PR #9460, we need to merge that one too. Hopefully, @ChrisMarsh82 will review/merge PR #9460 today.

[N-thony]

@ChrisMarsh82 are you also happy with this?