Feedback from alpha-test

[marcboulle]

Khisto API - Référence

Accès à la documentation

Actuellement, l’accès se fait de façon peu intuitive :

• sur le dépôt, chercher à droite « Deployments », puis aller sur « github-pages », et enfin suivre l’URL « Latest deployments > Last deployed ».

Je vois plusieurs problèmes :

• La documentation devrait être plus accessible, bien en évidence.
• Le README est largement redondant avec la documentation, ce qui peut être perturbant et source d’erreurs.

Je pense que le README devrait être plus succinct, en mettant en avant le lien vers la documentation, sans redondance (plus d’installation ni de quick start dans le README). Il pourrait simplement présenter des vues d’histogrammes de base à titre de teaser.

Documentation

Khisto — Histograms that fit your data

Très bien, avec les trois lignes et les deux figures illustratives

Get started

Je garderais le plan du README (ancienne version):

• Installation (avec les deux lignes pip)
• Quick start
  • option 1
    • API NumPy : minimaliste, avec la Gaussienne, comme dans la doc (quatre lignes)
    • Intégration avec Matplotlib : minimaliste, reproduction en quelques lignes deux figures illustratives
  • option 2, vraiment minimaliste (je préfère)
    • uniquement le code permettant la reproduction en quelques lignes deux figures illustratives

Plan sur la droite

Je mettrais le menu Core en dernier, après Histograms et Matplotlib.

Menu Histograms: pourqoui a-t-on deux sous-menus khisto.array.histogram.histogram et khisto.array.histogram.api

Menu Matplotlib: OK

Menu Core: cf. ci-dessous

API NumPy-like

Très bien.

Intégration avec Matplotlib

La description des paramètres semble chargée, avec un mélange :

• des paramètres spécifiques à khisto
• des paramètres par défaut de Matplotlib
• certains paramètres décrits sans explication claire

Serait-il possible de :

• se concentrer sur les paramètres obligatoires spécifiques à khisto (x, max_bins, ax, etc.)
• s’inspirer de la documentation d’Astropy (https://docs.astropy.org/en/stable/api/astropy.visualization.hist.html)
• utiliser kwargs pour transmettre les paramètres standards de Matplotlib
• compléter ou préciser la documentation pour les paramètres non supportés

Il est aussi important de signaler que, dans le contexte de khisto, la densité est la visualisation privilégiée. En effet :

• Avec des histogrammes à largeur fixe, la différence entre un histogramme de comptage et de densité est invisible.
• Avec des binings adaptatifs, la différence visuelle est importante, notamment pour représenter une Gaussienne.

Je suggère même que le paramètre density soit activé par défaut :

• pour éviter des visualisations peu lisibles par défaut
• en précisant que ce comportement diffère de celui de Matplotlib, et en documentant cette différence
• à discuter selon votre choix

Noyau (Core engine)

khisto.core.compute_histograms : OK

khisto.core.HistogramResult : OK

• Attributs :
  • Pourquoi sont-ils définis deux fois, une fois avec le type et la description, et une autre dans un tableau avec un ordre différent ?
  • La seconde description dans un tableau est redondante et pourrait être simplifiée. Peut-être que sa suppression serait compliquée ?

khisto.core.backend

Je ne comprends pas pourquoi cette documentation est présente (elle semble redondante avec compute_histograms et HistogramResult).

Démo

On commence par un exemple très complexe :

• Peut-être trop pour une introduction didactique
• Les points isolés sont noyés dans la masse et ne seront pas isoléss
• Pour qu’ils soient mis en évidence sous forme de bins de type Dirac, il faudrait qu’ils soient répétés

Une page didactique est en cours de finalisation sur le site principal Khiops :

• Lien vers la documentation
• Elle présente des histogrammes allant du plus simple au plus complexe
• À terme, on pourrait faire un lien vers cette page?

Il faudrait au minimum un exemple très simple, comme ceux de la Gaussienne ou de Pareto, même si ces cas sont abordés dans une section "Quick start".

Suggestion

• partir des histogrammes de type Gaussienne et Pareto, avec un exemple minimaliste
  • visualisation de base de la densité, en échelle linéaire pour la Gaussienne et log x log pour la Pareto
• utiliser un exemple plus complexe mais raisonnable, pour illustrer l'API comme cela est fait actuellement
  • une Gaussienne standard, plus une Gaussienne piquée, plus une lognormale standard
  • 10000 point pour avoir des bins plus précis)

data = np.concatenate([
    rng.normal(0, 1, 5000),
    rng.normal(3, 0.25, 2000),
    rng.lognormal(mean=0, sigma=1, size=3000)
])

• visualisation comparative avec les histogrammes matplotlib, comme pour la Gaussienne et la Pareto
  • en échelle standard
  • en échelle log log
• puis reprendre le reste des exemples de la démo, tels quels (mais avec l'exemple plus simple)

En visualisation

Il est presque indispensable de privilégier la densité, et non le nombre d’instances par bin.

Détail

• Eviter les paramètre de type color="#B8C4D6", préférer les noms de couleurs explicites

Installation

Le temps de l'alpha, il fallait utiliser la commande

pip install --extra-index-url https://test.pypi.org/simple/ khisto

Plus option -U si package existant déjà installé.

Installation et désinstallation

Essais combinatoire d'installation et désinstallation:

• installation et désinstallation d'un des deux package, ou des deux consécutivement (khisto et "khisto[matplotlib]")
• installation d'un package et déinstallation de l'autre
• installation plusieurs fois d'un package
• désinstallation plusieurs fois d'un package
• ...

Bilan:

• tout fonctionne correctement, y compris dans les "unhappy paths"
• le package "khisto[matplotlib]" est effectivement plus lourd, en raison des dépendance matplotlib
• surprise, ou non?
  • désinstaller un ou l'autre des packages à exactement le même effet

Question potentielle (l'état actuel me va):

• est-il vraiment justifié d'avoir deux options de packaging?
• sachant que l'usage principal, et de loin, et le plotting

Tests

Test élémentaire

Après avoir tout désinstallé, puis installé uniquement khisto (sans la partie plotting),
j'arrive quand même à utiliser la fonction khisto.matplotlib.hist ???
Est-ce normal?

Test complets

Test de la demo: OK

Tests intensifs, ceux du feedback de la première alpha: OK (plus de bug rencontré)

Test d'effets de bord

import numpy as np
from khisto import histogram

def test_side_effects():
    # Test of side effects datasets
    gaussian_data = np.random.normal(0, 1, 100)
    for data in [gaussian_data, [], [0], [0, 0], [0, 1], [0, 1, 2, 3, 4], [np.nan], [0, np.nan], ["a"], [[0]], [[0], [1]], [[[0]]]]:
        print("Histogram " + str(data))
        print("\tnumpy")
        try:
            hist, bin_edges = np.histogram(data)
            print(hist)
            print(bin_edges)
        except Exception as e:
            print(f"An error occurred while computing the numpy histogram: {e}")
        print("\tkhisto")
        try:
            hist, bin_edges = histogram(data)
            print(hist)
            print(bin_edges)
        except Exception as e:
            print(f"An error occurred while computing the khisto histogram: {e}")

Comportement: OK

• les exceptions sont bien remontées
• suggestion mineure: améliorer le message de l'exception, en cas de tableau vide, avec ou sans avoir filtré les valeurs manquantes
  • message actuel: An error occurred while computing the khisto histogram: Input array is empty after filtering
  • message souhaité si tableau initial vide: An error occurred while computing the khisto histogram: Input array is empty
  • message souhaité si tableau initial vide après filtrage des valeurs manquantes: An error occurred while computing the khisto histogram: Input array is empty after filtering missing values

[ElouenGinat]

Hello Marc,

I’ve made all the requested changes.

I reworked the README, and the link to the documentation is now clearly visible. I also changed the README quickstart section to highlight option 2.

In the documentation, I moved Core to the end of the list.

Regarding the hist (Matplotlib) issue, I reworked the entire implementation to make it closer to the Matplotlib API and to allow the use of **kwargs instead of reimplementing every parameter individually. With this approach, only the bins argument is passed to the Matplotlib API, which means that density and counts are now recomputed by Matplotlib. I also made density=True the default for hist, but not for the NumPy API.

For Core, I still don’t know how to remove the duplicated attribute section. It’s the same issue for the backend page — Sphinx is quite difficult to manage.

I also added clearer warnings for empty arrays.

Finally, I reworked the demo based on your feedback.

As for the rest of the feedback, it relates to pip packaging rather than to khisto specifically. For now, I would prefer to keep Matplotlib as an optional extra.