-
Notifications
You must be signed in to change notification settings - Fork 2
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
make quantreg run with statistical uncertainty #143
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #143 +/- ##
==========================================
+ Coverage 65.12% 65.56% +0.44%
==========================================
Files 17 17
Lines 1832 1838 +6
==========================================
+ Hits 1193 1205 +12
+ Misses 639 633 -6 ☔ View full report in Codecov by Sentry. |
@kemccusker The labor SCCs are now replicating and this PR is ready for review again. |
@@ -318,6 +318,8 @@ def scc(): | |||
self.logger.info("Processing SCC calculation ...") | |||
if self.fit_type == "quantreg": | |||
self.full_uncertainty_iqr | |||
self.calculate_scc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not clear to me why we call full_uncertainty_iqr
and stat_uncertainty_iqr
here. Are we assuming we'll want to calculate both anything we have a "quantreg" fit type? I don't think that's necessarily true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure exactly how to work with this since it's kind of a weird quirk of the order_plate
function. In the original scc
function which is a "course" of the order_plate
function, it will only calculate full_uncertainty_iqr
(if quantreg). This meant that if I wanted statistical uncertainty, I couldn't go through the order_plate
function. There are a few potential solutions I can think of:
- Add a new attribute to dscim that specifies uncertainty range (i.e. stat or full)
- Replace this scc function with a function that calls recipe functions according to which
save_files
you have specified - Replace the two uncertainty functions with a single function that feeds the scc (collapsed for stat uncertainty or uncollapsed for full uncertainty) into it based on
fair_aggregation
- Add a few more "courses" that allow the user to select which type of scc they want
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's try the first option "Add a new attribute to dscim that specifies uncertainty range (i.e. stat or full)" in a later PR
src/dscim/menu/main_recipe.py
Outdated
@save(name="stat_uncertainty_iqr") | ||
def stat_uncertainty_iqr(self): | ||
"""Calculate the distribution of quantile-weighted SCCs produced from | ||
quantile regressions collapsed across pulse dimension. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"collapsed across pulse dimension" is a bit confusing. This IQR is taken across only the quantreg quantile dimension because there is no climate uncertainty dimension. This is trickier with RFF-SPs and we need to test it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's reword "quantile regressions that have already been collapsed across other dimensions to give statistical-only uncertainty".
In a separate PR, we should nicely fail if someone tries to run quantreg with RFF-SPs.
No description provided.