Open
Conversation
lsabor
commented
Nov 13, 2025
Contributor
Author
There was a problem hiding this comment.
For review purposes, don't worry about the math functions regarding splines outside of the Command class.
Reviewers can probably start around line 293
hlbmtc
reviewed
Dec 3, 2025
hlbmtc
reviewed
Dec 3, 2025
hlbmtc
reviewed
Dec 3, 2025
hlbmtc
reviewed
Dec 3, 2025
hlbmtc
reviewed
Dec 3, 2025
hlbmtc
reviewed
Dec 3, 2025
hlbmtc
reviewed
Dec 3, 2025
…reshape-continuous-command
…reshape-continuous-command
Contributor
📝 WalkthroughWalkthroughAdds a new Django management command to reshape continuous questions (rescale and optionally convert to discrete), implements C2 cubic-spline utilities and solvers for distribution transforms, updates forecasts in batches, and adds a ModelBatchCreator for bulk_create batching. Changes
Sequence Diagram(s)sequenceDiagram
participant Cmd as Command
participant PostQ as Post/Question
participant Forecast as Forecasts
participant Spline as CubicSpline
participant Batch as ModelBatchCreator
participant Agg as Aggregation
Cmd->>PostQ: (optional) clone post & question
Cmd->>Forecast: clone forecasts for new question
Cmd->>Spline: build C2 spline from original distribution
Cmd->>Spline: evaluate transform_cdf at target points
Cmd->>Batch: enqueue transformed forecast objects
Batch->>Forecast: bulk_create / bulk_update in batches
Cmd->>Agg: rebuild question aggregations
Cmd->>Forecast: optionally trigger rescore
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
utils/models.py (1)
376-390: Inheritance bypasses parent__init__— consider composition or a shared base.
ModelBatchCreatorextendsModelBatchUpdaterbut never callssuper().__init__()and re-declares all attributes manually. The only value from the inheritance is reusingappend,__enter__, and__exit__. This works, but a reader might mistakenly assume the parent'sfieldsattribute is available. A cleaner approach would be to extract the shared logic into a common base class.Given the PR note that this code need not be highly polished, this is fine for now.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/models.py` around lines 376 - 390, ModelBatchCreator inherits from ModelBatchUpdater but never calls super().__init__, re-declares attributes, and can mislead readers about inherited state (e.g., fields); either call super().__init__() from ModelBatchCreator.__init__ to properly initialize parent state or refactor shared attributes and methods (append, __enter__, __exit__, fields) into a small common base class that both ModelBatchCreator and ModelBatchUpdater extend or use composition to hold a shared helper instance—update ModelBatchCreator.__init__ to delegate initialization accordingly and remove duplicated attribute declarations.questions/management/commands/reshape_continuous_question.py (1)
480-489: Continuous CDF is not renormalized — may produce out-of-range or non-monotonic values.In the discrete branch (lines 528-536), the PMF is carefully renormalized. In the continuous branch, the raw spline output is used directly. A cubic spline can overshoot, producing values < 0 or > 1, and can also produce non-monotonic output (since the spline is fit to a CDF but monotonicity isn't enforced). This could yield invalid CDFs that break downstream aggregation or scoring.
Consider clamping and enforcing monotonicity:
🛡️ Proposed fix
if not discrete: # evaluate cdf at critical points new_cdf: list[float] = [] for x in np.linspace( new_nominal_range_min, new_nominal_range_max, new_inbound_outcome_count + 1, ): location = scaled_location_to_unscaled_location(x, basis_question) new_cdf.append(get_cdf_at(location)) + # Clamp and enforce monotonicity + new_cdf = [max(0.0, min(1.0, v)) for v in new_cdf] + for i in range(1, len(new_cdf)): + new_cdf[i] = max(new_cdf[i], new_cdf[i - 1])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@questions/management/commands/reshape_continuous_question.py` around lines 480 - 489, The continuous-path CDF values collected into new_cdf (via scaled_location_to_unscaled_location and get_cdf_at) are not renormalized or constrained and may be non-monotonic or outside [0,1]; after computing new_cdf enforce a valid CDF by (1) clamping raw values to a reasonable numeric range, (2) forcing monotonicity (e.g., replace new_cdf with a non-decreasing transform such as cumulative maximum), and (3) renormalizing so the first value equals 0 and the last equals 1 (subtract the first element and divide by the final element) before using the array downstream to match the discrete branch behavior. Ensure these steps are applied where new_cdf is built so subsequent code expects a proper CDF.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@questions/management/commands/reshape_continuous_question.py`:
- Around line 400-412: The success/warning reporting references idx which is
unbound if question.user_forecasts is empty; initialize idx = 0 before the loop
(above the for over question.user_forecasts) so the variable always exists, keep
the loop logic that sets/uses idx as before (inside the ModelBatchCreator block
with Forecast, creator.append, etc.), and then use that idx in the final
self.stdout.write(self.style.SUCCESS(...)) call to safely report "Copied {idx}
forecasts... DONE" even when there are zero forecasts.
- Around line 117-119: The sub- and super-diagonal arrays A_lo and A_hi are
undersized and cause IndexError when _solve_tridiagonal reads a[i] for i in
range(1, n+1) and when the clamped-BC path writes A_lo[n]; change their
allocation in reshape_continuous_question.py to length n+1 (e.g., A_lo = [0.0] *
(n + 1) and A_hi = [0.0] * (n + 1)) so all indexed accesses (including A_lo[n]
and reads up to a[n]) are valid, and update any comments to reflect the new
storage layout used by _solve_tridiagonal and the clamped boundary-condition
code paths.
- Around line 710-714: The rescore branch currently calls
score_question(question, question.resolution) which rescoring the original
object; when --alter_copy is used the modified object is question_to_change, so
change the call to target question_to_change and its resolution (e.g.,
score_question(question_to_change, question_to_change.resolution)) and ensure
you only call this when rescore is true and question_to_change is not None;
update any surrounding logic that assumes the original to use question_to_change
for rescoring.
- Around line 579-588: The current defaulting uses question.range_min/max
(padded real range) for nominal_range_min/nominal_range_max, which
double-applies the half-step for discrete questions; change the default
computation so when options["nominal_range_min"] or options["nominal_range_max"]
is None you compute the nominal values (un-padded) instead of using
question.range_min/max. Concretely: locate the
nominal_range_min/nominal_range_max assignments and, if the question is discrete
(or has a step), compute step (use existing default step logic or derive it from
inbound_outcome_count if step isn’t yet available), then set nominal_range_min =
question.range_min + 0.5 * step and nominal_range_max = question.range_max - 0.5
* step (or derive nominal bounds directly from inbound_outcome_count for integer
steps); otherwise leave the existing fallback for continuous questions. Ensure
this uses the same symbols (options["nominal_range_min"],
options["nominal_range_max"], question.range_min, question.range_max, step,
inbound_outcome_count, and reshape_question) so the later half-step adjustment
in reshape_question is not applied twice.
- Around line 684-699: The try/except around reshape_question inside the
transaction.atomic block swallows exceptions and allows partial commits; update
the error handling so exceptions trigger rollback by either re-raising the
caught exception after logging or moving the try/except outside the
transactional context. Specifically, in the block that calls
self.reshape_question(...) ensure you do not return on exception—call
self.stdout.write(...) then raise the exception (or restructure so
transaction.atomic encapsulates the call and the logging/return happens after
the with block) so that transaction.atomic can roll back partial writes.
- Around line 320-324: The --rescore flag is inverted because
parser.add_argument(..., action="store_false") makes options["rescore"] False
when the flag is passed; change this so the flag behavior matches its name: if
you want "opt-in to rescoring", change the argument to use action="store_true"
(keep the dest/options key as "rescore") and ensure code that checks
options["rescore"] (e.g., the check at the location referenced as line ~710 that
reads options["rescore"]) uses that truthy value to run rescoring;
alternatively, if the intent is "rescore by default, allow disabling", replace
the flag with --no_rescore (use dest="no_rescore" and an action that sets it to
False when passed) and update the downstream check to use options["no_rescore"]
(e.g., change the conditional at the existing check to if not
options["no_rescore"] and question.resolution is not None:).
- Around line 156-221: The not-a-knot branch contains dead code and an
IndexError: remove the unused tridiagonal setup (A_diag, A_hi, rhs modifications
and the if n == 1 … else … pass block) and implement an explicit
early-return/fallback when n == 1 before building the dense system—e.g. detect n
== 1, set the natural boundary M (or call the existing natural path) and return
CubicSplineC2(x, y, M, bc="natural") so the subsequent dense construction that
accesses h[1] never runs; otherwise keep the dense matrix build and call
_dense_gauss_solve(A, b) and return CubicSplineC2(x, y, M, bc="not-a-knot").
- Around line 546-558: The loop in the reshape_continuous_question management
command can leave idx undefined when forecasts.iterator yields zero items,
causing an UnboundLocalError; initialize idx (e.g., idx = 0) before the for-loop
that iterates forecasts.iterator (the block that updates
forecast.continuous_cdf, forecast.distribution_input and calls updater.append)
or otherwise guard the final self.stdout.write with a check using the forecasts
count so the final log uses a defined value; update the code around the for idx,
forecast in enumerate(forecasts.iterator(chunk_size=100), 1) loop (and
references to idx in the final success message) to ensure idx is always defined.
---
Nitpick comments:
In `@questions/management/commands/reshape_continuous_question.py`:
- Around line 480-489: The continuous-path CDF values collected into new_cdf
(via scaled_location_to_unscaled_location and get_cdf_at) are not renormalized
or constrained and may be non-monotonic or outside [0,1]; after computing
new_cdf enforce a valid CDF by (1) clamping raw values to a reasonable numeric
range, (2) forcing monotonicity (e.g., replace new_cdf with a non-decreasing
transform such as cumulative maximum), and (3) renormalizing so the first value
equals 0 and the last equals 1 (subtract the first element and divide by the
final element) before using the array downstream to match the discrete branch
behavior. Ensure these steps are applied where new_cdf is built so subsequent
code expects a proper CDF.
In `@utils/models.py`:
- Around line 376-390: ModelBatchCreator inherits from ModelBatchUpdater but
never calls super().__init__, re-declares attributes, and can mislead readers
about inherited state (e.g., fields); either call super().__init__() from
ModelBatchCreator.__init__ to properly initialize parent state or refactor
shared attributes and methods (append, __enter__, __exit__, fields) into a small
common base class that both ModelBatchCreator and ModelBatchUpdater extend or
use composition to hold a shared helper instance—update
ModelBatchCreator.__init__ to delegate initialization accordingly and remove
duplicated attribute declarations.
Contributor
🚀 Preview EnvironmentYour preview environment is ready!
Details
ℹ️ Preview Environment InfoIsolation:
Limitations:
Cleanup:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
questions/management/commands/reshape_continuous_question.py (1)
383-384: Typo in parameter name:appove_copy_post(missingr).Consistent across the method signature (line 383), the variable in
handle(line 576), and the call site (line 672) — but still reduces readability.♻️ Proposed rename
- def make_copy_of_question( - self, question: Question, appove_copy_post: bool - ) -> Question: + def make_copy_of_question( + self, question: Question, approve_copy_post: bool + ) -> Question:And in
handle:- appove_copy_post = options["approve_copy_post"] + approve_copy_post = options["approve_copy_post"] ... - stored_question = self.make_copy_of_question(question, appove_copy_post) + stored_question = self.make_copy_of_question(question, approve_copy_post)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@questions/management/commands/reshape_continuous_question.py` around lines 383 - 384, Rename the misspelled parameter appove_copy_post to approve_copy_post throughout the method signature and all usages: update the method definition (the function whose signature reads "self, question: Question, appove_copy_post: bool) -> Question"), the reference inside handle, and the call site currently passing appove_copy_post so names match; update any type hints, docstrings, and local variable usages to approve_copy_post to keep consistency and avoid NameError/readability issues.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@questions/management/commands/reshape_continuous_question.py`:
- Around line 529-534: The renormalization can divide by zero when
np.sum(ip_array) == 0; update the block around ip_array, prob_below_lower and
prob_above_upper so you first compute total = np.sum(ip_array) and only divide
when total > 0, otherwise allocate the remaining mass (1 - prob_below_lower -
prob_above_upper) uniformly across the ip_array buckets (or set them to zeros if
uniform allocation is inappropriate), then continue to build new_cdf; reference
the ip_array variable and the prob_below_lower/prob_above_upper values and
ensure new_cdf is produced from a non-NaN/Inf ip_array.
- Around line 454-459: The current logic in reshape_continuous_question.py
assigns question_to_change.scheduled_resolve_time whenever
new_scheduled_resolve_time or new_scheduled_close_time is truthy, causing
scheduled_resolve_time to be overwritten when only --new_scheduled_close_time is
supplied; change the condition so scheduled_resolve_time is only updated when a
new_scheduled_resolve_time is explicitly provided (e.g., check
new_scheduled_resolve_time is not None or truthiness depending on how values are
represented) and leave the scheduled_resolve_time untouched when only
new_scheduled_close_time is passed, keeping the existing assignment to
question_to_change.scheduled_close_time as-is.
- Around line 310-313: The --alter_copy flag is silently ignored when used
without --make_copy because handle falls back to question_to_change = question;
add an early validation in the handle method to detect when
options.get("alter_copy") is True while options.get("make_copy") is False and
raise a clear error (e.g., CommandError or parser.error) informing the user that
--alter_copy requires --make_copy; reference the --alter_copy and --make_copy
flags and the handle function to locate where to add this guard so the command
fails fast instead of silently modifying the original question.
- Around line 600-608: The code that converts date-type nominal_range_min and
nominal_range_max uses datetime.fromisoformat(...).timestamp(), which interprets
dates in local time and yields wrong UTC timestamps; update the exception
handlers for nominal_range_min and nominal_range_max (the parsing logic around
those variables) to parse ISO date strings with an explicit UTC timezone (e.g.,
call datetime.fromisoformat(...).replace(tzinfo=dt_timezone.utc).timestamp())
similar to the scheduled-time parsing, so the resulting values are correct UTC
Unix timestamps; ensure you import or reference dt_timezone (or
datetime.timezone.utc) consistently with the surrounding parsing code.
---
Duplicate comments:
In `@questions/management/commands/reshape_continuous_question.py`:
- Around line 703-708: The rescore block currently calls
score_question(question, ...) which rescales the original instead of the edited
copy when --alter_copy is used; change the call to
score_question(question_to_change, question_to_change.resolution) (or pass the
corresponding resolution variable from question_to_change) so that the copy gets
rescored; update any surrounding logic that assumes question is being modified
to use question_to_change for rescoring when question_to_change is set and only
use question when no copy was created.
- Around line 117-119: A_lo and A_hi are undersized causing IndexError because
clamped BC writes A_lo[n] and _solve_tridiagonal expects a[i] for i in 1..n;
change their initialization to allocate n+1 entries (like A_diag) so indices
0..n are valid (update A_lo = [0.0] * (n+1) and A_hi = [0.0] * (n+1)) so the
clamped BC and _solve_tridiagonal access patterns are safe.
- Around line 546-558: The loop uses idx in the final "DONE" log but idx is
unbound if forecasts.iterator yields nothing; before the for-loop in
reshape_continuous_question (the block using "with ... as updater") initialize
idx = 0, and after exiting the with-updater block (i.e., move the
self.stdout.write(self.style.SUCCESS(...)) that prints the DONE message outside
the with) emit the final "Rescaled {idx}/{c} forecasts... DONE" so it runs after
the updater has flushed; keep the transform_cdf(forecast.continuous_cdf),
forecast.distribution_input = None, and updater.append(forecast) logic
unchanged.
- Around line 156-221: The not-a-knot branch leaves the n == 1 fallback without
returning, so execution continues and h[1] is accessed causing IndexError;
modify the branch in the bc == "not-a-knot" block to immediately return the
natural-spline result for n == 1 (i.e., compute/leave M as the natural case or
reuse the existing natural fallback logic and then return CubicSplineC2(x, y, M,
bc="natural")/or CubicSplineC2(x, y, M, bc="not-a-knot") as appropriate) so the
dense-system path (which accesses h[1]) is never reached when n == 1; update the
early-branch to reference n, h, A_diag/rhs or reuse the natural-case solver and
return before building A/b.
- Around line 400-412: The loop uses idx after the for-loop and assumes it
exists and also prints the final success message while still inside the
ModelBatchCreator context; to fix, initialize a counter (e.g., count = 0) before
entering the with ModelBatchCreator and increment it inside the loop (or set idx
= 0 before the loop) so it's defined when the queryset is empty, and move the
final success message (the self.stdout.write(self.style.SUCCESS(...))) to after
the with ModelBatchCreator block so __exit__ can flush the final batch before
the "DONE" message; reference ModelBatchCreator, Forecast,
question.user_forecasts.iterator, idx/__exit__ in the change.
- Around line 579-588: The defaults for nominal_range_min/max should use the
question's nominal (unpadded) values instead of the padded question.range_min/
range_max so reshape_question doesn't pad twice; change the default logic in
reshape_continuous_question to use options["nominal_range_min"] if provided,
otherwise compute nominal_range_min = question.range_min + 0.5 * question.step
(and nominal_range_max = question.range_max - 0.5 * question.step) or use an
existing question.nominal_* property if available, then pass those nominal
values into reshape_question so no extra padding is applied.
---
Nitpick comments:
In `@questions/management/commands/reshape_continuous_question.py`:
- Around line 383-384: Rename the misspelled parameter appove_copy_post to
approve_copy_post throughout the method signature and all usages: update the
method definition (the function whose signature reads "self, question: Question,
appove_copy_post: bool) -> Question"), the reference inside handle, and the call
site currently passing appove_copy_post so names match; update any type hints,
docstrings, and local variable usages to approve_copy_post to keep consistency
and avoid NameError/readability issues.
cemreinanc
temporarily deployed
to
testing_env
GitHub Actions
Inactive
cemreinanc
temporarily deployed
to
testing_env
GitHub Actions
Inactive
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
questions/management/commands/reshape_continuous_question.py (1)
503-507: Replaceassertwith an explicit check in production code.
assertstatements are stripped underpython -O. Ifstring_location_to_bucket_indexunexpectedly returnsNone, execution silently continues intopmf[None], producing aTypeErrorwith no context.♻️ Proposed refactor
- assert index is not None + if index is None: + raise ValueError( + f"Location {x!r} has no bucket in basis_question {basis_question.id}" + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@questions/management/commands/reshape_continuous_question.py` around lines 503 - 507, The code currently uses assert index is not None after calling string_location_to_bucket_index(str(round(x, 10)), basis_question), which can be skipped under python -O and leads to a confusing TypeError when indexing pmf with None; replace the assert with an explicit runtime check: call string_location_to_bucket_index as before, then if index is None raise a descriptive exception (e.g., ValueError) or log an error including basis_question and the rounded x value before raising, and only then append pmf[index] to inbound_pmf to ensure clear, deterministic failure handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@questions/management/commands/reshape_continuous_question.py`:
- Around line 666-670: The command currently only checks if step is None when
convert_to_discrete is requested, allowing zero or negative values that later
cause a ZeroDivisionError in reshape_question; update the validation in the
management command (reshape_continuous_question.py) where convert_to_discrete
and step are handled to require step to be a positive number (step > 0), emit a
clear error via self.stdout.write(self.style.ERROR(...)) when step is <= 0, and
return early—this ensures reshape_question will never receive an invalid step
and avoids the division-by-zero.
- Around line 461-462: Replace the direct nullable access
question_to_change.post with the safe helper question_to_change.get_post() and
handle the None case the same way as earlier in this file (see the existing
pattern around line 392): call get_post(), check if it returned a Post, and only
then call post.update_pseudo_materialized_fields(); if get_post() returns None,
skip the update or raise the same error/handling used elsewhere to keep behavior
consistent.
---
Duplicate comments:
In `@questions/management/commands/reshape_continuous_question.py`:
- Around line 579-588: The default nominal_range_min/max are being set to
question.range_min and question.range_max (which are already padded for discrete
steps), causing padding to be applied twice when reshape_question adds
±0.5*step; change the defaults to use the unpadded nominal values
(question.nominal_range_min and question.nominal_range_max) instead of
question.range_min/question.range_max so reshape_question's padding is only
applied once.
- Around line 400-412: The UnboundLocalError occurs because idx is only assigned
inside the for loop in reshape_continuous_question when iterating
question.user_forecasts, so if the queryset is empty the final
self.stdout.write(f"Copied {idx} forecasts... DONE") references an undefined
variable; fix it by initializing idx = 0 before the for loop that uses
ModelBatchCreator (the block creating Forecast objects and appending to creator)
so idx is always defined, then keep the existing progress logging using idx,
making no other structural changes to ModelBatchCreator, Forecast, new_question,
new_post, or the self.stdout.write calls.
- Around line 310-313: Add a validation check that prevents --alter_copy from
being used without --make_copy: after parsing args (the parser that defines the
"--alter_copy" and "--make_copy" flags) detect if args.alter_copy is true while
args.make_copy is false and call parser.error(...) (or raise a clear exception)
with a helpful message; ensure this check runs before any code that sets
question_to_change (the variable assigned to question when alter_copy is
misused) so the command fails fast instead of silently modifying the original.
- Around line 710-714: The rescore branch currently rescopes the original
`question` instead of the potentially reshaped copy `question_to_change`; change
the condition and call to use `question_to_change` (i.e., check `rescore and
question_to_change.resolution is not None` and call
`score_question(question_to_change, question_to_change.resolution)`) so that
when `--alter_copy` is used the copied/reshaped question is rescored rather than
the original; keep the same `score_question` function and ensure you reference
`question_to_change` for both the guard and the argument.
- Around line 546-562: The final success log uses the loop variable idx which is
only set inside the for-loop, causing UnboundLocalError when
forecasts.iterator() yields nothing; ensure idx is defined before the loop
(e.g., initialize idx = 0) or compute a safe count to log after the loop, then
use that safe variable in the f-string in the Rescaled ... DONE message; update
the block around forecasts.iterator(...)/updater to initialize idx (or use a
separate processed_count) and reference that instead of idx, leaving
transform_cdf, updater.append(forecast) and
build_question_forecasts(question_to_change) unchanged.
- Around line 454-459: The current logic assigns
question_to_change.scheduled_resolve_time to new_scheduled_close_time when only
--new_scheduled_close_time is provided; change this so scheduled_resolve_time is
only updated when new_scheduled_resolve_time is explicitly supplied.
Specifically, in the block using new_scheduled_resolve_time and
new_scheduled_close_time, replace the condition "if new_scheduled_resolve_time
or new_scheduled_close_time:" with a check for only new_scheduled_resolve_time
and set question_to_change.scheduled_resolve_time = new_scheduled_resolve_time;
keep the separate assignment of question_to_change.scheduled_close_time using
new_scheduled_close_time so close time updates remain unaffected.
- Around line 117-119: A_lo and A_hi are undersized and cause IndexError because
_solve_tridiagonal reads indices up to n and the clamped branch writes to
A_lo[n]; change their allocation to length n+1 (e.g., A_lo = [0.0] * (n+1) and
A_hi = [0.0] * (n+1)) and update the inline comments to reflect that A_lo stores
sub-diagonal entries for indices 1..n and A_hi stores super-diagonal entries for
indices 0..n-1/1..n as used by _solve_tridiagonal and the clamped BC handling.
- Around line 529-534: The renormalization step can divide by zero when all
inbound_pmf buckets are zero; in the function handling inbound_pmf
(reshape_continuous_question), compute total = np.sum(ip_array) and if total ==
0.0 avoid the division — set ip_array to zeros (or a safe uniform/epsilon
distribution if you prefer) and proceed so you don't produce NaN/Inf in new_cdf;
otherwise perform the existing (1 - prob_below_lower - prob_above_upper) *
ip_array / total normalization.
---
Nitpick comments:
In `@questions/management/commands/reshape_continuous_question.py`:
- Around line 503-507: The code currently uses assert index is not None after
calling string_location_to_bucket_index(str(round(x, 10)), basis_question),
which can be skipped under python -O and leads to a confusing TypeError when
indexing pmf with None; replace the assert with an explicit runtime check: call
string_location_to_bucket_index as before, then if index is None raise a
descriptive exception (e.g., ValueError) or log an error including
basis_question and the rounded x value before raising, and only then append
pmf[index] to inbound_pmf to ensure clear, deterministic failure handling.
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
3 participants
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.
closes #3707
adds command:
python manage.py reshape_continuous_questionparams:
This command will be rarely (hopefully essentially never) used and the code doesn't need to be polished. But it should be checked over for any logic faults.
Summary by CodeRabbit
New Features
Improvements