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

Validate signal and background integral values in plot_DetPlot() #206

Merged
merged 1 commit into from
Sep 6, 2024
Merged

Validate signal and background integral values in plot_DetPlot() #206

merged 1 commit into from
Sep 6, 2024

Conversation

mcol
Copy link
Contributor

@mcol mcol commented Sep 6, 2024

By enforcing that signal.integral.max > signal.integral.min we also avoid that the computation of the number of channels produces Inf (fixes #203).

Locally, running coverage on this produces this error from the test that uses multicore = 1:

Error in `checkForRemoteErrors(val)`: one node produced an error:
 could not find function ".validate_positive_scalar"

That error would go away if I used Luminescence:::.validate_positive_scalar, but that seems to be against the CRAN policy. Alternatively, one could move the validation checks before the self-call.

But in the meantime, let's see if CI is happy with it as it is.

By enforcing that signal.integral.max > signal.integral.min we also avoid
computing Inf for n.channels.
@mcol
Copy link
Contributor Author

mcol commented Sep 6, 2024

Ok, this is fine! I'm not sure what's going on in my session (also a restart didn't help), but we can go ahead with this.

@RLumSK
Copy link
Member

RLumSK commented Sep 6, 2024

@mcol What is against the CRAN policy is the call with ::: but this is only needed for the checks locally. When the package is build the function has access to the namespace after access outside with :::.

@RLumSK RLumSK merged commit 7c90b0d into R-Lum:dev_0.9.x Sep 6, 2024
10 checks passed
@mcol mcol deleted the issue_203 branch September 6, 2024 09:50
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.

2 participants