Enable number field diagram search via computed discriminant axes (LMFDB#6985)#34
Open
roed-math wants to merge 4 commits into
Open
Enable number field diagram search via computed discriminant axes (LMFDB#6985)#34roed-math wants to merge 4 commits into
roed-math wants to merge 4 commits into
Conversation
The diagram search axis options are built from SearchColumns whose name is a
real database column. Number fields expose a "disc" column (a MultiProcessedCol
combining the separately-stored disc_sign and disc_abs), which is not a database
column, so it was filtered out of the axis choices -- yet it was the configured
x_axis_default, making the default diagram request 500 with KeyError: 'disc'.
The page was consequently disabled via NFSearchArray.has_diagram = False.
Add a reusable "computed_cols" option to diagram_opts (handled in
SearchWrapper._diagram_search): a mapping from an axis key to {label, cols, func,
type} that registers axes whose values are derived from one or more database
columns via a function. Number fields use it for the signed discriminant
(disc = disc_sign * disc_abs) and the absolute discriminant, and re-enable the
diagram page. Also make the empty-result path hand the template an empty list
rather than None, and fall back to the raw axis key for unknown axis labels, so
degenerate cases degrade cleanly. Pages that don't set computed_cols are
unaffected.
Verified with the flask test client: default and explicit signed/absolute
discriminant axes render correct plots, existing axes (rd/regulator/num_ram) and
the elliptic-curve/groups diagrams are unchanged, empty result sets and
single-value axes return 200. Added test_diagram_search; full number_fields
test suite passes; pyflakes clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
havarddj
reviewed
Jul 24, 2026
| jump_knowl = "nf.search_input" | ||
| jump_prompt = "Label, name, polynomial, or comma-separated list" | ||
| has_diagram = False | ||
| has_diagram = True |
There was a problem hiding this comment.
This is superfluous, has_diagram is true by default.
Owner
Author
There was a problem hiding this comment.
Done — removed in c0019dc; has_diagram defaults to True via getattr(self, "has_diagram", True).
havarddj
reviewed
Jul 24, 2026
| # discriminant is a plain column that can be selected as an axis. | ||
| "computed_cols": { | ||
| "disc": { | ||
| "label": "Discriminant", |
There was a problem hiding this comment.
"Discriminant" (and "Absolute discriminant") below should be lower case to be consistent with the other labels.
Owner
Author
There was a problem hiding this comment.
Done in 1bb8aa9 — both labels are now lowercase ("discriminant", "absolute discriminant"), matching the sort labels.
havarddj
reviewed
Jul 24, 2026
havarddj
left a comment
havarddj
left a comment
There was a problem hiding this comment.
Apart from the slightly cosmetic comments above, this looks great to me!
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
The number field diagram search was disabled because choosing discriminant as an axis did
not work: the database stores the sign and absolute value of the discriminant in separate
columns, so the signed discriminant is not a plain column and could not be selected — and
the configured default even made the page 500 with
KeyError: 'disc'. This adds a reusablecomputed_colsoption todiagram_opts(handled inSearchWrapper._diagram_search) thatregisters axes whose values are derived from one or more database columns via a function,
adds those columns to the projection, and evaluates the function when building the diagram
data. Number fields use it for the signed discriminant (
disc_sign * disc_abs) and theabsolute discriminant, and the diagram page is re-enabled. Empty result sets and unknown
axis keys now degrade cleanly instead of erroring, and pages that do not set
computed_colsare unaffected. Verified with the flask test client (default/signed/absolute discriminant
axes render correct plots, existing axes and the elliptic-curve/groups diagrams unchanged,
degenerate cases return 200); added
test_diagram_search; number_fields tests and pyflakespass. Addresses LMFDB#6985.
🤖 Generated with Claude Code