Skip to content

Normalize real roots returned by CurveFunction - #32

Merged
nritschel merged 2 commits into
masterfrom
31-normalize-real-output-of-curvefunction
Sep 23, 2026
Merged

nritschel merged 2 commits into
masterfrom
31-normalize-real-output-of-curvefunction

Conversation

@SybelBlue

@SybelBlue SybelBlue commented Sep 22, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #31

Summary

This PR aligns CurveFunction.get_x_for_yval with its declared types:

  • normalize accepted roots from CurveFunction.get_x_for_yval() to built-in float values
  • use the configured imaginary-component tolerance when filtering complex results
  • add regression coverage for returning the expected real value while omitting complex roots

Testing

Added a regression test and passed:

  • uv run pytest -q
  • uv run ruff check .
  • uv run pyright

@SybelBlue
SybelBlue marked this pull request as ready for review September 22, 2026 21:58
@SybelBlue
SybelBlue requested a review from nritschel September 22, 2026 21:59

@nritschel nritschel left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think there's a bug in your current change (see comment). Fixing it should be a matter of just replacing the comparison of xvals and tolerance to tvals. The rest looks good.

Comment thread sketchresponse/grader_lib/CurveFunction.py Outdated
@nritschel
nritschel self-requested a review September 23, 2026 19:18

@nritschel nritschel left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good now after the fix!

@nritschel
nritschel merged commit f8f30d3 into master Sep 23, 2026
5 checks passed
@nritschel
nritschel deleted the 31-normalize-real-output-of-curvefunction branch September 23, 2026 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CurveFunction.get_x_for_yval() returns zero-imaginary complex values that crash np.arctan2

2 participants