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

New: functions underlaying paper "A novel tool to access crosstalk in luminescence detection" Anna-Maartje de Boer, Luc Steinbuch #560

Merged
merged 16 commits into from
Feb 7, 2025

Conversation

LucSteinbuch
Copy link
Contributor

See email

@mcol
Copy link
Contributor

mcol commented Feb 5, 2025

Thanks Luc! I've launched our continuous integration tests, but I've realised that they will probably fail. What's required is to run devtools::document() from your repository and commit the changes to NAMESPACE and and the documentation files under the man directory (only those concerning your new functions, you should ignore the rest).

Are you comfortable with that? Otherwise I'll make those changes myself if I'm allowed to push to your branch.

@LucSteinbuch
Copy link
Contributor Author

LucSteinbuch commented Feb 5, 2025 via email

@RLumSK
Copy link
Member

RLumSK commented Feb 5, 2025

@mcol: You have maintainer rights, so you should be able to make those changes.

@mcol
Copy link
Contributor

mcol commented Feb 5, 2025

I've made the minimum number of changes to make the package compile with the new functions, let's see if we remain green or if I missed something. Next I'll start adding some tests.

@mcol mcol force-pushed the new_crosstalk_detection branch from a9c6583 to 8b1d137 Compare February 6, 2025 10:15
@mcol
Copy link
Contributor

mcol commented Feb 6, 2025

OK, I've added tests to cover the new functions (only a few lines are still missing from complete coverage). As far as I'm concerned, this is ready to be merged on master. We'll iterate further from there, I'll follow up by email.

@RLumSK If you are happy too, please do the merge as Luc's commit is not signed.

@LucSteinbuch Next time (we really hope it will be soon!) we'll ask you to make sure that your commits are verified according to this doc: https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification (I forgot that we have this requirement).

Copy link
Member

@RLumSK RLumSK left a comment

Choose a reason for hiding this comment

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

I suggest merging the calc_MoransI_xxx() functions into one calc_MoransI() that then has an option "method" to tap into the different approaches. This would declutter a little bit the documentation and number of exported functions.

Perhaps the best approach is to branch out this functions into a separate file called calc_MoransI.R otherwise it will be hard to find the functions and it somewhat breaks the logic how functions are implemented in the package. The rules to apply here:

  • Each exported functions has its own .R file.
  • If helper functions are required they can go either into the same file with a name that is preceded by an . (e.g., .get_something <- function ...) or we safe them in the internals_RLum.R file.
  • Exceptions are always possible, however, they should be truly justified.

R/plot_Crosstalk.R Outdated Show resolved Hide resolved
R/apply_Crosstalk.R Outdated Show resolved Hide resolved
@mcol
Copy link
Contributor

mcol commented Feb 6, 2025

I've applied some of the changes suggested. I think we should liaise with @LucSteinbuch as per changes to the exported functions, and that's what I meant to iterate on once an initial version is merged.

@mcol
Copy link
Contributor

mcol commented Feb 7, 2025

I've renamed some of the function arguments as follows:

  • renamed bo_return_inbetween_numbers to return_intermediate_values
  • removed all the bo_ prefixes

mcol added 3 commits February 7, 2025 11:28
This introduces the spatial_autocorrelation option, which can be set to
FALSE to reproduce the behaviour of calc_MoransI_expt_no_cor().
This introduces the compute_pseudo_p, tested_MoransI and n_permutations
arguments to control the computation of pseudo p-values.
@mcol mcol force-pushed the new_crosstalk_detection branch from dd07380 to c4fc940 Compare February 7, 2025 14:37
@mcol
Copy link
Contributor

mcol commented Feb 7, 2025

OK, all the changes we discussed are now implemented, so I think this can now be merged.

Add roxygen2 tags and remove FIXME(mcol)
Revert part of the previous change
@RLumSK RLumSK added this to the Release version 1.0.0 milestone Feb 7, 2025
@RLumSK RLumSK merged commit 25968f7 into R-Lum:master Feb 7, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants