Bug fix for PR(30) for 29 savedata cannot take in datatable objects

gslab-econ / gslab_r

5 stars 1 forks source link

Bug fix for PR(30) for 29 savedata cannot take in datatable objects #33

Closed zhizhongpu closed 2 months ago

zhizhongpu commented 2 months ago

Here we address the issues with PR #30 including:

fatal: cannot load function https://github.com/gslab-econ/gslab_r/pull/30#issuecomment-2223648712
uniqueN bugs for numeric columns
did not format large numbers using comma separators e.g. 123,456
did not round decimals to 3 digits after .

zhizhongpu commented 2 months ago

@liaochris once you join, please add yourself as a reviewer. After you confirm that 1) the code runs without problems, and 2) the goals outlined in #29, #30, and #33 are accomplished, please approve and I'll merge to main. Ideally I'd like to do this soon to minimize the impact on other projects. Thank you.

fyi @veli-m-andirin

zhizhongpu commented 2 months ago

@liaochris previously we experienced no problems when running in RStudio interactive session BUT is.data.table and other data.table dependencies were not loaded when we ran from terminal, either using scons or rscript.

This bug should be fixed by 46cc4e9d91e53d6848faebc15e7f3b77d17d67a9. On my end, I see that rscript command works on my terminal and produces the desired SaveData output. (Rstudio still works too)

Please verify that it works for you too. Thank you.

liaochris commented 2 months ago

@liaochris previously we experienced no problems when running in RStudio interactive session BUT is.data.table and other data.table dependencies were not loaded when we ran from terminal, either using scons or rscript.

This bug should be fixed by 46cc4e9. On my end, I see that rscript command works on my terminal and produces the desired SaveData output. (Rstudio still works too)

Please verify that it works for you too. Thank you.

@zhizhongpu This works when I test it on my local, using "Rscript" --no-save --no-restore --verbose source/derived_large/alerts_clean/impute_countries.R

zhizhongpu commented 2 months ago

@liaochris one optional thing that you can do here if you have bandwidth is to: clean up unnecessary modules from data.table that are imported in SaveData.R and NAMESPACE. A few modules may be unnecessary and ideally should be removed, either by you or myself (which I plan to do in #31 ).

liaochris commented 2 months ago

@zhizhongpu Are we supposed to edit NAMESPACE by hand? If not, how do we update it?

liaochris commented 2 months ago

@zhizhongpu Are we supposed to edit NAMESPACE by hand? If not, how do we update it?

NVM figured it out, use devtools::document()

liaochris commented 2 months ago

@zhizhongpu Can you resolve the merge conflicts on the PR first?

zhizhongpu commented 2 months ago

thanks @liaochris

@veli-m-andirin I just tested the latest branch both in RStudio and on terminal (using rscript) and everything looks great. Please approve if you have no problems with the state of this branch by 709ae3d2944ff3ed4b350eebd543b8b25a94fd79 - thank you.