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

clean ups in RejectionCriteria returned from analyse_SAR.TL() #245

Closed
mcol opened this issue Sep 16, 2024 · 3 comments · Fixed by #246
Closed

clean ups in RejectionCriteria returned from analyse_SAR.TL() #245

mcol opened this issue Sep 16, 2024 · 3 comments · Fixed by #246
Labels
cosmetic issue Not a bug, but it looks nicer if changed
Milestone

Comments

@mcol
Copy link
Contributor

mcol commented Sep 16, 2024

As part of #147, I'm having a detailed look at what analyse_SAR.TL() produces and returns. One of this is the rejection.criteria data frame, which is sometimes produced incorrectly.

Example:

data(ExampleData.BINfileData, envir = environment())
object <- Risoe.BINfileData2RLum.Analysis(TL.SAR.Data, pos = 3)

analyse_SAR.TL(
        list(object, object),
        signal.integral.min = 210,
        signal.integral.max = 220,
        dose.points = 1:7,
        integral_input = "temperature",
        sequence.structure = c("SIGNAL", "BACKGROUND"))

res@data$rejection.criteria
#             citeria value threshold status
# 1 recuperation rate    NA   +/- 0.1   <NA>   # recycling ratio
# 2 recuperation rate    NA       0.1     OK
# 3 recuperation rate    NA   +/- 0.1   <NA>   # recycling ratio
# 4 recuperation rate    NA       0.1     OK

In these lines of code:

RejectionCriteria <- data.frame(
citeria = c(colnames(RecyclingRatio), "recuperation rate"),
value = c(RecyclingRatio,Recuperation),
threshold = c(
rep(paste("+/-", rejection.criteria$recycling.ratio/100)
,length(RecyclingRatio)),
paste("", rejection.criteria$recuperation.rate/100)
),

  • Typo: "citeria" instead of "criteria", but actually "criterion" would be better, as the other column names are singular
  • Since RecyclingRatio is NA, colnames(RecyclingRatio) is NULL, so we get that "recuperation rate" is printed in all the rows (due to R doing recycling the non-null element in the vector): I would make it write "recycling ratio" when RecyclingRatio is NA
  • The docs say that for rejection.criteria NA is produced if no R0 dose point exists, but that's not the case, as the above data frame is generated
  • The "status" column actually reports 0.1 (with a space) rather than 0.1 due to the paste("") term
  • Bonus: the +/- could be replaced with the unicode symbol ± if desired

In these other lines:

if(is.na(Recuperation)==FALSE &
Recuperation>rejection.criteria$recuperation.rate){"FAILED"}else{"OK"}

  • While value is NA in all cases, in the "recycling ratio" cases the status is NA, but for "recuperation rate" the status is OK: I suppose it should be NA in all cases, but perhaps that difference in treatment was intentional?
@mcol mcol added the cosmetic issue Not a bug, but it looks nicer if changed label Sep 16, 2024
@mcol mcol added this to the Release version 1.0.0 milestone Sep 16, 2024
@RLumSK
Copy link
Member

RLumSK commented Sep 16, 2024

I do agree with the suggested changes, thank you! And no, this NA vs OK mixture was not intentionally.

@mcol
Copy link
Contributor Author

mcol commented Sep 16, 2024

One additional thing: for recuperation rate we report the rejection criteria divided by 100 (line 358), but we don't divide in the actual test where we set the status (lines 369-370). For recycling ratio we divide in both places, so I expect that the same would be correct for recuperation rate too.

This means that until now recuperation rates have been marked OK in more cases than expected.

@RLumSK
Copy link
Member

RLumSK commented Sep 16, 2024

OK, obviously this also needs a fix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cosmetic issue Not a bug, but it looks nicer if changed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants