search

IDEMSInternational / R-Instat

A statistics software package powered by R
http://r-instat.org/
GNU General Public License v3.0
38 stars 102 forks source link

Refactored RSyntax #8881

Closed lloyddewit closed 6 months ago

lloyddewit commented 6 months ago

@rdstern @N-thony Please ignore for now, thanks

lloyddewit commented 6 months ago

@rdstern In PR #8869 I am working on a proof-of-concept. The most efficient first step is to simplify some of the current classes. This could also benefit the current version of R-Instat. This PR contains a refactored version of RSyntax. More refactoring is possible but this would increase the risk of regression and require significant testing. For this PR, all the dialogs should work exactly as before. Some suggested dialogs to test are below. If these still work correctly, then all the other dialogs should also be fine:

@N-thony I removed about 300 lines of unused code and made some other small improvements. I also put the data members and functions in the correct order. I realise now that the reordering has made the RSyntax diff bigger and your peer review more complex. Sorry for that. The diffs are not as big as they appear, many of the functions are unchanged and have just been moved. I tried hard to keep the refactoring as low risk as possible, but obviously RSyntax is one of the core classes. If you think it's necessary, then you could also ask another of the senior developers to review.

Thank you!

rdstern commented 6 months ago

@lloyddewit this is exciting.

a) I followed your suggestions and started with the boxplot dialog. I used the usual survey data and it worked fine. It seemed to work better than before when I tried the plot options button. Was that my imagination? It used to be very slow in going to the sub-dialog the first time, and it seems to be quicker now. I was going to ask @N-thony to look at the code, but maybe don't need to?

b) Then I fitted a simple 2-variable model: image

That's so I could test the dlgTwoVariableUseModel dialog:

That gives an error, as follows:

image

c) I started again, but am not sure whether I am testing the dlgModelling function. Is it the Fit > Model > General?

image

If so, my initial tests it seems to work fine.

lloyddewit commented 6 months ago

@rdstern Thank you for testing.

a)

I would love to take credit for the perceived speed increase but I doubt it was my change :)

b)

I was able to recreate the error in this PR but I was also able to recreate the error in master branch. Are we sure that the error is due to this PR's changes?

c) I started again, but am not sure whether I am testing the dlgModelling function. Is it the Fit > Model > General

You can access the dlgModelling via:

image

N-thony commented 6 months ago

@lloyddewit this is exciting.

a) I followed your suggestions and started with the boxplot dialog. I used the usual survey data and it worked fine. It seemed to work better than before when I tried the plot options button. Was that my imagination? It used to be very slow in going to the sub-dialog the first time, and it seems to be quicker now. I was going to ask @N-thony to look at the code, but maybe don't need to?

b) Then I fitted a simple 2-variable model: image

That's so I could test the dlgTwoVariableUseModel dialog:

That gives an error, as follows:

image

c) I started again, but am not sure whether I am testing the dlgModelling function. Is it the Fit > Model > General?

image

If so, my initial tests it seems to work fine.

@lloyddewit @rdstern I was also to reproduce the error in the master branch. It seems like we don't have get_models method anymore, I checked in one of my old branch and found the method there but I'm ignoring why we don't have it anymore in the current version. This is the method in on of my oldest branch image

lloyddewit commented 6 months ago

@rdstern For part b) we could check the R code in the log file that is generated by the dialog. If the R code generated by master branch is the same as the R code generated by this PR, then this PR has not caused any regression. Would this be an adequate test to approve this PR? Obviously the error still needs fixing but this should be done in a separate issue/PR. What do you think?

N-thony commented 6 months ago

@rdstern For part b) we could check the R code in the log file that is generated by the dialog. If the R code generated by master branch is the same as the R code generated by this PR, then this PR has not caused any regression. Would this be an adequate test to approve this PR? Obviously the error still needs fixing but this should be done in a separate issue/PR. What do you think?

I think this bug is related to the work done recently by @Patowhiz.

Patowhiz commented 6 months ago

@N-thony, please note that the get_models R function has been deprecated and replaced with get_object_data. This change is documented in detail in issue #7808, where I've explained the new concept for processing output objects.

If you encounter any dialogs still generating get_models, they need to be updated. I believed all model dialogs had been refactored to use the new get_object_data function, so any remaining instances of get_models are unexpected and should be corrected.

rdstern commented 6 months ago

@N-thony I hope the discovery of the get_models R-Instat function is the cause of the apparent regression in the modelling dialog. Should there be a new issue on this topic or do we continue here? I assume there could be lurking get_models elsewhere in the modelling dialog, so this is a good time to check more broadly?

N-thony commented 6 months ago

@N-thony I hope the discovery of the get_models R-Instat function is the cause of the apparent regression in the modelling dialog. Should there be a new issue on this topic or do we continue here? I assume there could be lurking get_models elsewhere in the modelling dialog, so this is a good time to check more broadly?

@rdstern yes, I would suggest that @Vitalis95 works on this with support from @Patowhiz

lloyddewit commented 6 months ago

@rdstern If the get_models is the only known problem (to be fixed in a separate PR), then is this PR ready to merge or does it need more testing? Thanks

lloyddewit commented 6 months ago

@N-thony I think I just found an error. It looks like it is caused by this PR. Please could you confirm the error? If you can recreate the error and agree that it is caused by this PR, then please could you revert the merge? I am very sorry for the inconvenience.

@N-thony @rdstern I found an issue in the Enter dialog. If I try and add a new column containing then the correct R code is not generated. Details below. I will investigate the error tomorrow.

image

image

Expected script (from 0.7.16):

image

Script from this PR:

image