fix(matplotlib): corriger le rendu des histogrammes adaptatifs - #35
ElouenGinat wants to merge 11 commits into
Conversation
Réutiliser les fréquences Khiops pour les valeurs extrêmes et rendre visibles les classes très étroites par défaut. Refuser également le type barstacked non pris en charge. Closes #34
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The cumulative density implementation, ec edge styling, and stale notebook import remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (2)
What changed in this PR
Updates the Matplotlib histogram adapter to preserve Khiops frequencies, improve adaptive-bin rendering, and expose hist at the package root.
Changes:
- Adds typed histogram returns and rejects unsupported stacked histograms.
- Updates plotting behavior, tests, documentation, CI, changelog, and demo metadata.
| File | Summary |
|---|---|
tests/plot/test_matplotlib_histogram.py |
Adds histogram behavior and typing tests. |
src/khisto/matplotlib/hist.py |
Implements frequency-backed plotting and return handling; cumulative density behavior and ec styling require changes. |
src/khisto/__init__.py |
Exports hist at the package root. |
sandbox/khisto_demo.ipynb |
Updates demo metadata; retains a stale _hist import that causes an import error. |
pyproject.toml |
Adds the Python 3.10 typing dependency. |
docs/index.rst |
Updates usage examples. |
docs/conf.py |
Configures Matplotlib API documentation. |
docs/api_comparison.md |
Documents unsupported histogram options. |
CHANGELOG.md |
Records the histogram changes. |
.github/workflows/ci.yaml |
Adds optional-dependency installation coverage. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ct for unequal-width bins' Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
marcboulle
left a comment
marcboulle
left a comment
There was a problem hiding this comment.
J'ai bien aimé la simplification de la démo ;)
Pour le reste, je n'ai fait que survolé le code rapidement. Je ne comprend pas tous les détails et le pourquoi des évolutions. C'est suite aux retours utilisateurs? ou à des tests approfondis?
J'ai quelques remarques et surtout des questions de détail.
There was a problem hiding this comment.
Le rendu de khisto.histogram est un peu bizarre sur un navigateur:
Passer à plus simplement khisto.histogram ?
| "\n", | ||
| "> 📚 Go further with [Histograms - Khiops](https://khiops.org/learn/histograms/),\n", | ||
| "a didactic walk-through from the simplest histogram to the most complex." | ||
| "Khisto uses the **Khiops optimal binning algorithm** to choose both the number and\n", |
There was a problem hiding this comment.
L'introduction est meilleure qu'avant ;)
| values, edges, patches = ax.hist( | ||
| bin_edges[:-1].tolist(), | ||
| bin_edges.tolist(), | ||
| weights=frequencies.tolist(), |
There was a problem hiding this comment.
Solution alternative au précédent np.nextafter, plus simple et plus robuste?
| if histtype == "bar" and "edgecolor" not in kwargs: | ||
| if not isinstance(histogram_patches, BarContainer): | ||
| raise TypeError("Matplotlib unexpectedly returned non-bar patches.") | ||
| for patch in histogram_patches.patches: |
There was a problem hiding this comment.
Selon ma compréhension, tu forces un edgecolor de même couleur que facecolor si ce paramètre n'est pas utilisé, pour forcer un épaisseur minimale aux bins:
- mais comment-est pris en compte alors que c'est fait après l'appel à
ax.hist? - quid si on a mis paramétré
linewidthà 0? (pas de problème après tout?) - quid si on a mis paramétré
edgecolorà blanc? (on ne verra pas le bin, qui restera d'épaisseur nulle?)
Davantage commenter le code: sans le contexte, on ne comprend pas l'intention.
What
histtype="barstacked".histfrom the package root and infer its return type fromhisttype.Validation
uv run pytest -quv run sphinx-build -W --keep-going -b html docs docs/_build/htmlCloses #34