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

Add Verbose Argument to forder Function for Enhanced Logging Control (#4533) #6179

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Nj221102
Copy link
Contributor

@Nj221102 Nj221102 commented Jun 12, 2024

closes #4533

Description

This pull request addresses issue #4533 by adding a verbose argument to the forder function. This enhancement allows for better control over the verbosity of output, particularly when forder is called internally by other functions. This is useful for managing detailed logging in higher-level functions without overwhelming the output with messages from forder.

  • This feature builds upon the logging enhancements implemented in #4491.

Checklist

  • Added verbose argument to forder function.
  • Updated internal calls to propagate verbose argument.
  • Adjusted logging mechanism to respect verbose level.
  • Added tests for verbose functionality.

References

Implementation of the Verbose Argument in forder

Initially, I added a simple verbose argument to the forder function and set the verbose variable like this:

const int Verbose = INTEGER(verbose)[0];

This straightforward approach allowed us to control the verbosity of forder by using a command such as forder(verbose = verbose - 1L). However, this implementation revealed a significant issue: many tests started failing because forder no longer used the GetVerbose() function to retrieve the global verbosity settings. This change caused problems in scenarios where the global verbosity setting was expected to control the output.

For instance, when we attempted the following command:

DT[order(C)[1:5], B, verbose=2L]

we did not receive the verbose output from forder as anticipated. This issue arose because the forder function was solely relying on the local verbose argument, ignoring the global verbosity settings controlled by GetVerbose().

Resolving the Issue: Combining Global and Local Verbosity

To address this problem and allow forder to respect both the global verbosity setting and the local verbose parameter, I modified the implementation using a ternary operator. This operator determines which verbosity value should be used based on the global setting and the local argument:

const int verbose = GetVerbose() >= 2 ? GetVerbose() - 1L : INTEGER(verboseArg)[0];

With this modification, users can set the verbosity to a value greater than or equal to 2L to get verbose output from forder. The logic behind this approach is as follows:

  1. Global Verbosity Control: If the global verbosity setting, retrieved using GetVerbose(), is greater than or equal to 2, the forder function will use this value. This ensures that the global setting takes precedence and reduces by one to adjust for the internal logic.

  2. Local Verbosity Control: If the global verbosity setting is less than 2, the forder function will use the local verbose argument provided by the user. This allows for specific control over the verbosity for individual function calls.

Benefits of the Combined Approach

This combined approach offers several advantages:

  • Flexibility: Users can control verbosity at both a global and a local level, providing greater flexibility in managing output.
  • Backward Compatibility: Existing code that relies on global verbosity settings continues to function as expected.

In summary, this improvement ensures that forder respects both global and local verbosity settings, allowing for comprehensive and flexible control over verbose output. This change resolves the issue where verbose output was not being generated as expected and ensures that all verbosity controls work harmoniously.

src/forder.c Outdated Show resolved Hide resolved
test(2098.5, DT[do.call(order, mget(groups)), verbose=TRUE], ans)
test(2098.6, DT[with(DT, do.call(order, mget(groups))), verbose=TRUE], ans)
test(2098.7, DT[do.call(forder, mget(groups)), verbose=TRUE], ans)
test(2098.8, DT[with(DT, do.call(forder, mget(groups))), verbose=TRUE], ans)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Include a brief comment explaining the differences among the tests

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what type of difference are we talking about ?, these test were already there and i just modified them because their output changed due to this pr, Should i leave a comment informing this change??

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, a comment describing that change and/or what these set of tests do might be good to have.

@Rdatatable Rdatatable deleted a comment from github-actions bot Jun 13, 2024
Copy link

github-actions bot commented Jun 13, 2024

Comparison Plot

Generated via commit 9ccb2db

Download link for the artifact containing the test results: ↓ atime-results.zip

Time taken to finish the standard R installation steps: 12 minutes and 37 seconds

Time taken to run atime::atime_pkg on the tests: 3 minutes and 23 seconds

@Nj221102 Nj221102 marked this pull request as ready for review June 13, 2024 14:57
@@ -288,10 +288,11 @@ int checkOverAlloc(SEXP x)
}

SEXP alloccolwrapper(SEXP dt, SEXP overAllocArg, SEXP verbose) {
if (!IS_TRUE_OR_FALSE(verbose))
error(_("%s must be TRUE or FALSE"), "verbose");
if ((!isLogical(verbose) && !isInteger(verbose)) || LENGTH(verbose)!=1 || INTEGER(verbose)[0]==NA_INTEGER)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if condition is too terse to be readable and relies on the undocumented behaviour of INTEGER() on logical input.

With integer verbose values, we should document the integer codes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have the same condition here:

data.table/src/init.c

Lines 319 to 321 in ff900d1

if ((!isLogical(opt) && !isInteger(opt)) || LENGTH(opt)!=1 || INTEGER(opt)[0]==NA_INTEGER)
error(_("verbose option must be length 1 non-NA logical or integer"));
return INTEGER(opt)[0];

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah this if condition is already being used in many parts of the code base where verbose can be both INTEGER and Logical

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that tells me we need a helper function :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thoughts!

@Nj221102 Nj221102 marked this pull request as draft June 24, 2024 08:39
@Nj221102 Nj221102 marked this pull request as draft June 24, 2024 08:39
@Nj221102 Nj221102 marked this pull request as draft June 24, 2024 08:39
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.

C forder could take verbose argument
5 participants