-
Notifications
You must be signed in to change notification settings - Fork 22
mc/3841/setup/non-prediction-values #3842
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
base: main
Are you sure you want to change the base?
Conversation
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.
Is there a better way to handle this? None of these instances should actually ever trigger. Should I just handle their values down the line by "value || 0.0" for instance?
It might be possible to create different Question types each with unique Forecast values, so we could skip having to verify there are no "null" values for continuous questions. I'm unsure if that makes things more complicated with little benefit, though.
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 assume in this place we’re just rendering data coming from the backend, so there’s no actual validation happening. In this line we throw an error that blocks further rendering, and I don’t think that’s useful for the user since they can’t do anything about it
support null values on frontend fix flake8 complaints
597d325 to
c734f33
Compare
| def get_medians( | ||
| forecasts: list[Forecast | AggregateForecast], | ||
| ) -> list[AggregationEntry]: | ||
| medians = [] | ||
| timesteps: set[float] = set() | ||
| for forecast in forecasts: | ||
| timesteps.add(forecast.start_time.timestamp()) | ||
| if forecast.end_time: | ||
| timesteps.add(forecast.end_time.timestamp()) | ||
| for timestep in sorted(timesteps): | ||
| prediction_values = [ | ||
| f.get_pmf() | ||
| for f in forecasts | ||
| if f.start_time.timestamp() <= timestep | ||
| and (f.end_time is None or f.end_time.timestamp() > timestep) | ||
| ] | ||
| if not prediction_values: | ||
| continue # TODO: doesn't account for going from 1 active forecast to 0 | ||
| median = np.median(prediction_values, axis=0) | ||
| predictors = len(prediction_values) | ||
| medians.append( | ||
| AggregationEntry(median, predictors if predictors > 1 else 0, timestep) | ||
| ) | ||
| return medians |
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.
unused
hlbmtc
left a comment
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.
Lgtm from the backend side, but want @ncarazon to check & comment on the frontend part
| cdf: historyItem.forecast_values.map((v) => { | ||
| if (v === null) { | ||
| throw new Error("Forecast values contain null values"); | ||
| } | ||
| return v; | ||
| }), |
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’m not totally sure about throwing an error here – is it necessary to hard-fail if there’s a null in forecast_values?
If we do want this guard, could we move the check higher up so we don’t have to duplicate it in every chart?
closes #3841
support for None in forecast values