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

report mode when simulation error occurs #329

Open
wants to merge 24 commits into
base: master
Choose a base branch
from

Conversation

rtesra
Copy link
Collaborator

@rtesra rtesra commented Aug 11, 2022

This PR updates the extract_indicators() and output_package() function to return mode indicator values when simulating indicators fails. Changes made:

  • Added a class to the object returned by extract_indicators() to indicate if the mean or mode has been returned
  • Added an additional step in output_package() that will remove sample if simulation error occurs and generate indicators with only mean values. This will also output a naomi warning that will be displayed to users on the web app to indicate that uncertainty was not generated.

@jeffeaton At the moment this does not output the original warning from extract_indicators() indicating which indicator caused the simulation fail. Options are:

  • Output as a warning in naomi scripts - this is useful for debugging purpose
  • Output as warning on web app - less useful as it does not give users much of an indication of the source of the problem

@rtesra rtesra requested a review from jeffeaton August 11, 2022 16:31
@jeffeaton
Copy link
Collaborator

Code for producing example:

devtools::load_all()

source("~/Downloads/naomi-v2.6.9/tests/testthat/setup-model-frame.R")

a_fit_bad_sample <- a_fit_sample
a_fit_bad_sample$sample$anc_alpha_t2_out[1,] <- NA_real_

out_bad <- output_package(a_fit_bad_sample, a_naomi_data) out_good <-
output_package(a_fit_sample, a_naomi_data)

# For sample with NAs, mode is returned and mean is NA
expect_true("mode" %in% class(out_bad$indicators))
expect_true(all(is.na(out$indicators$mean)))

# For sample without NAs, mode and mode returned expect_true("mean" %in%
class(out_good$indicators))
expect_true(all(!is.na(out_good$indicators$mean)))

Copy link
Collaborator

@jeffeaton jeffeaton left a comment

Choose a reason for hiding this comment

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

Thanks for this, this looks generally good to me.

A comment on the structure: I think that returning mode field if no uncertainty generated and mean field if is generated will be an undue headache for the front end. In principle, I like keeping this separate, but I suspect it will be far less hassle for @r-ash if we copy the mode field into the mean field for display purposes.

Let's discuss with @r-ash + also confirm with him that missing values for lower / upper doesn't create any issues for plotting, etc.

Note to update NEWS and bump version number.

R/outputs.R Outdated Show resolved Hide resolved
tests/testthat/test-model-fit.R Outdated Show resolved Hide resolved
@jeffeaton
Copy link
Collaborator

@rtesra -- regarding this comment:

@jeffeaton At the moment this does not output the original warning from extract_indicators() indicating which indicator caused the simulation fail. Options are:

Output as a warning in naomi scripts - this is useful for debugging purpose
Output as warning on web app - less useful as it does not give users much of an indication of the source of the problem

I not completely sure what you are referring to with naomi scripts? Can you clarify? But I agree it's not so useful to return to the user at the moment.

@jeffeaton jeffeaton marked this pull request as draft September 8, 2022 10:57
@rtesra rtesra requested a review from jeffeaton September 15, 2022 13:36
@rtesra rtesra marked this pull request as ready for review September 15, 2022 13:36
Copy link
Collaborator

@jeffeaton jeffeaton left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good to me. Thanks for example on the warnings generation.

I've approved this; I'm holding merging it because I'm not sure if @r-ash is ready for it yet.

tests/testthat/test-outputs.R Show resolved Hide resolved
@rtesra
Copy link
Collaborator Author

rtesra commented Sep 16, 2022

hintr build hivtools/hintr#404

@r-ash
Copy link
Contributor

r-ash commented Sep 22, 2022

Run up the app and still some issues in calibrate so this is on hold

@rtesra rtesra requested a review from jeffeaton September 23, 2022 13:56
@rtesra rtesra added the bug Something isn't working label Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants