Fix Issue #8: Transform continuous outcome estimates back to original scale#30
Draft
Fix Issue #8: Transform continuous outcome estimates back to original scale#30
Conversation
… scale - Modified estimate_pooled_results() to accept Qbounds and map_to_ystar parameters - Added transformation of final theta estimates from [0,1] scale back to original scale - Updated vim_numerics.R and vim_factors.R to pass bounds information - Added test script and documentation demonstrating the fix - Ensures continuous outcome estimates are reported on original scale, not [0,1]
- Added NEWS.md with entry for Issue #8 fix - Created comprehensive testthat test in test-continuous-outcome-scale.R - Test verifies estimates are on original scale, not [0,1] scale - Includes test for both continuous and binary outcomes - Removed old standalone test script
…mation The condition 'family == "binomial" && length(unique(Y)) > 2' was logically incorrect since binomial family should have binary outcomes (length(unique(Y)) <= 2). Simplified the condition to only check for 'family == "gaussian"' for continuous outcome transformation.
…nuous [0,1] outcomes The original condition 'family == "binomial" && length(unique(Y)) > 2' was correct. Binomial family can be applied to continuous outcomes in [0,1] range (quasibinomial). When there are >2 unique values, it indicates continuous outcome needing scale transformation.
- Allow both binomial (for continuous [0,1] outcomes) and gaussian families - Ensures continuous outcome rescaling works for both family types - Addresses feedback on original family condition logic
|
Looks like there are a few issues preventing this PR from being merged!
If you'd like me to help, just leave a comment, like
Feel free to include any additional details that might help me get this PR into a better state. You can manage your notification settings |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🎯 Overview
This PR addresses Issue #8 by implementing continuous outcome rescaling to transform variable importance estimates from the [0,1] scale back to the original outcome scale.
🔧 Problem
Variable importance estimates for continuous outcomes were being reported on the [0,1] scale instead of the original outcome scale, making results difficult to interpret. For example, if the original outcome ranged from 10-50, estimates were being reported as values between 0-1 rather than meaningful values in the 10-50 range.
💡 Solution
Core Changes
Modified
estimate_pooled_results():Qboundsandmap_to_ystarparameters to accept outcome bounds informationUpdated
vim_numerics()andvim_factors():Qboundsandmap_to_ystarparameters toestimate_pooled_results()Technical Implementation
The rescaling uses the formula:
Where
upper_boundandlower_boundare the original outcome variable's range.📝 Files Changed
R/estimate_pooled_results.R- Core rescaling logicR/vim-factors.R- Pass bounds for factor variable analysisR/vim-numerics.R- Pass bounds for numeric variable analysistests/testthat/test-continuous-outcome-scale.R- Comprehensive test suiteNEWS.md- Documentation of the fix🧪 Testing
Added comprehensive test suite that verifies:
The continuous outcome rescaling logic is implemented but currently has test failures where
vim$results_allis NULL. This suggests the rescaling changes may be interfering with VIM calculations and requires additional debugging to ensure the transformation doesn't break the core functionality.🎯 Next Steps
📚 Related