Logging improvements #9

FabrizioSandri · 2022-07-04T12:01:06Z

Description

The purpose of this pull request is to improve the logging system of RcppDeepState.
In this way developers that will be able to rapidly identify any issues that may arise using this library.

FabrizioSandri · 2022-07-04T12:23:57Z

With the last few commit I improved the logging system when some functions being analyzed contains as argument data types that are not allowed.

In detail, the function deepstate_pkg_create fails if developers try to run the TestHarness creation procedure on a library with functions containing in their signature (the argument list), data types which are not allowed with the following error message:

Testharnesses cannot be created for the package - datatypes fall out of specified list!!

This has been improved, printing for each function not analyzed, the list of parameters that fall out of the allowed list.

FabrizioSandri · 2022-07-05T08:32:35Z

Since importing RInside in the R code is not essential, I deleted it from the imports in the most recent commits. There is no need to import RInside because it is already there in the DESCRIPTION file, in the dependencies list. RInside will instead be imported and used inside the C++ code.

Additionaly I discovered that certain code parts require a little bit of review. For instance, there are parts of the code that are not used, such variable definitions. For example, I deleted the following line in the commit b012edb.

prototypes_calls <-deepstate_get_prototype_calls(package_path)

Because RcppDeepState is all about speed and performance to run the tests, even little adjustments like this one may have some impact.

FabrizioSandri · 2022-07-06T11:16:35Z

With the most recent updates, I added the verbose argument to various RcppDeepState methods. This option is used to offer more detailed information if it is set to TRUE. This parameter uses the value from the global option verbose by default.

FabrizioSandri · 2022-07-08T08:59:36Z

@tdhock do you think I need to make any other adjustments? I plan to review again the logging part later on.
To allow you to review these changes, I have sent you an invite to participate as a collaborator.

tdhock · 2022-07-08T16:59:11Z

.github/workflows/run_test.yaml

+    branches: [test-debug-symbols, logging-improvements]
  pull_request:
-    branches: [test-debug-symbols]
+    branches: [test-debug-symbols, logging-improvements]


is there some way to specify all branches?

Yes, I modified the workflow such that the "R CMD check" command is run on each branch.
Fixed in commit 6b85147

tdhock · 2022-07-08T17:00:19Z

R/analyze_binary.R

    }
    for(pkg.i in seq_along(test.files)){
-      list_testfiles[basename(test.files[[pkg.i]])] <- list(deepstate_analyze_fun(path,basename(test.files[[pkg.i]]),max_inputs))
+      list_testfiles[basename(test.files[[pkg.i]])] <- list(deepstate_analyze_fun(path,basename(test.files[[pkg.i]]), max_inputs, verbose))


please use double square brackets on left side of <- and remove list() from right side

I solved this in the commit 1d7e9cc

tdhock · 2022-07-08T17:00:51Z

R/get-pkgs.R

-
-

+  return(list(1,c()));


what is this?

explicit return is not needed at end of function, please remove.

I completely reviewed the entire function and changed it's name to deepstate_get_mismatched_datatypes. Now his function returns the list of mismatched arguments, instead of returning 0 or 1 alongside with the mismatched datatypes.

tdhock · 2022-07-08T21:24:08Z

R/get-pkgs.R

  datatypes <- list("NumericVector","NumericMatrix" ,"arma::mat","double",
                  "string","CharacterVector","int","IntegerVector")


this is the list of supported data types right? So rather than "mismatched" in the function name maybe "unsupported" would be more appropriate?
Also this list is redundant with the logic in https://github.com/FabrizioSandri/RcppDeepState/blob/master/R/fun_harness_create.R -- if you have time it would be good to reorganize that code so that a list or table is used to lookup what to do, rather than the current code which is rather difficult to understand (lots of if/else statements).

That's true, this list is redundant. I updated the deepstate_fun_create function inside fun_harness_create.R and replaced the if else statements with a table containing information for the supported datatypes. In detail each column of this table corresponds to a supported datatype and provides some details on it. The first row of this table contains an alternative datatype to be used when saving the inputs(e.g., using qs::c_qsave), whereas the second row correspond to the associated generation function that takes as input a range. When a datatype contains a value of NA in both rows, it means that is supported, utilizes itself when executing qs::c_qsave and lacks a range function. In this way checking if a datatype is supported is really simple: just check if exists a column for the datatype in this table.

I think the function deepstate_get_unsupported_datatypes may now be removed.

tdhock · 2022-07-12T17:46:32Z

for the list of supported types, instead of one column per type please try one row per type like this:

datatype <- function(ctype,rtype,args)data.table(ctype,rtype,args)
types_table <- rbind(
  datatype("int", "IntegerVector", "(low,high)"),
  datatype("double", "NumericVector", "(low,high)"))
setkey(types_table, ctype)
type_info <- types_table[my_type]
type_info$args

FabrizioSandri · 2022-07-15T13:21:38Z

@tdhock I replaced the list of supported datatypes with the types_table you mentioned in your previous comment and discussed in the Google meet of Tuesday.

tdhock · 2022-07-18T18:10:06Z

R/analyze_binary.R

    }
    for(pkg.i in seq_along(test.files)){
-      list_testfiles[[basename(test.files[[pkg.i]])]] <- deepstate_analyze_fun(path,basename(test.files[[pkg.i]]), max_inputs, verbose)
+      list_testfiles[[basename(test.files[[pkg.i]])]] <- deepstate_analyze_fun(path,basename(test.files[[pkg.i]]), max_inputs, verbose=verbose)


since you use basename(test.files[[pkg.i]]) twice please create an intermediate variable

Done! Updated in the commit 7b4d7e9

FabrizioSandri added 4 commits July 4, 2022 11:00

deepstate_datatype_check rewritten

593acf4

no need to use an else-if. Avoid to call deepstate_datatype_check twice

d42efaf

return the mismatched datatypes

9a44dfb

print the list of mismatched datatypes

0b4d2d9

FabrizioSandri added 2 commits July 5, 2022 10:16

removed unused code

ad4bf24

no need to import RInside

b012edb

FabrizioSandri added 3 commits July 6, 2022 11:32

verbose parameter

f76a7e1

roxygen documentation update

df138c9

documentation files

72f564b

Setup CI checks for this branch

f516aa0

FabrizioSandri mentioned this pull request Jul 6, 2022

First prototype FabrizioSandri/RcppDeepState-action#1

Merged

tdhock reviewed Jul 8, 2022

View reviewed changes

FabrizioSandri added 4 commits July 8, 2022 19:47

list_testfiles brackets fix

1d7e9cc

mismatched datatypes fix

91da805

workflow run on every branch

6b85147

mismatched datatypes improvement

1c33720

tdhock reviewed Jul 8, 2022

View reviewed changes

FabrizioSandri added 2 commits July 11, 2022 11:16

rename deepstate_get_unsupported_datatypes

8efdebc

deepstate_fun_create improvements - table

e8a1a60

data table update

22def71

verbose parameter fix

aa8639d

tdhock reviewed Jul 18, 2022

View reviewed changes

FabrizioSandri added 3 commits July 23, 2022 19:12

code refactoring

7b4d7e9

analyze binary parameters fix

95c3d88

run CI only once fix

cda836b

FabrizioSandri merged commit 7b2220d into master Jul 26, 2022

FabrizioSandri mentioned this pull request Aug 22, 2022

Rcpp string support #11

Open

		datatypes <- list("NumericVector","NumericMatrix" ,"arma::mat","double",
		"string","CharacterVector","int","IntegerVector")

Logging improvements #9

Logging improvements #9

Uh oh!

Conversation

FabrizioSandri commented Jul 4, 2022

Description

Uh oh!

FabrizioSandri commented Jul 4, 2022

Uh oh!

FabrizioSandri commented Jul 5, 2022

Uh oh!

FabrizioSandri commented Jul 6, 2022

Uh oh!

FabrizioSandri commented Jul 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FabrizioSandri Jul 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tdhock commented Jul 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FabrizioSandri commented Jul 15, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

FabrizioSandri Jul 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

FabrizioSandri Jul 11, 2022 •

edited

Loading

tdhock commented Jul 12, 2022 •

edited

Loading

FabrizioSandri Jul 23, 2022 •

edited

Loading