Skip to content

[ERT_IP] Débogage du script des histogrammes et update des fichiers associés#1

Open
Romadelf wants to merge 29 commits intohadrienmichel:masterfrom
Romadelf:dev
Open

[ERT_IP] Débogage du script des histogrammes et update des fichiers associés#1
Romadelf wants to merge 29 commits intohadrienmichel:masterfrom
Romadelf:dev

Conversation

@Romadelf
Copy link
Copy Markdown

@Romadelf Romadelf commented Jun 4, 2025

Ce github est super chouette et l'open source invite implicitement aux contributions communautaires. Dans la foulée de la rédaction du projet de prospec, j'ai débogué l'affichage des histogrammes et me suis dit "pourquoi pas ne pas contribuer en retour puisque le code est fait ?". Bon, le code du projet de mon groupe fait plus que ça, donc il fallait quand même revenir à une version minimaliste, par fidélité au fichier original. C'est l'objet principal de cette pull request, avec quelques modifs annexes explicités dans le changelog ci-dessous. Mais d'abord, parce qu'un joli dessin vaut mieux qu'un long discours :

 

[A] Avant cette pull request (ERT_IP/analyse.py avec ERT_IP/data/B52_DDN6_essai.ohm):

  • OK pour l'IP. L'axe vertical représente le nombre de mesures.
  • Pour l'ERT, les bins à droite sont sûr-représentés car il ne sont pas normalisés (density=False) malgré la grande plage
  • Les soucis de l'image correspondante de [B] s'appliquent en [A] aussi

 

[B] Avant cette pull request (code collé dans ERT_IP/README.md avec ERT_IP/data/B52_DDN6_essai.ohm):

  • L'axe vertical de l'IP est moins clair. Avec density=True, l'unité me semble devoir être interprétée "pourcentage du nombre de mesures par (mV/V)", ce qui n'est pas intuitif. L'utilisateur pourrait ne plus être certain que les proportions du nombre de mesures sont conservées (bien qu'on le voit ici via [A]).
  • Pour l'ERT, certes, les barres représentent plus fidèlement le nombre de mesures par ohm au travers de son pourcentage, mais :
    • Des 20 bins vraisemblablement souhaités selon le code (bug : il n'y en a que 19), seulement 8 sont visibles car les autres sont en dehors de la plage des résistances à gauche de la figure (par un autre bug)
      -> correction (density=False à droite) :

    • De plus, les résistances au delà de ~0.5 Ohm sont tronquées alors qu'elles vont jusque 5.35 Ohm et sont intéressantes vu cette correction additionnelle (density=False à droite pour rendre cette zone faible en densité de mesures visible à nouveau vu que son nombre absolu, lui, est considérable) :

    • Ces corrections souffrent d'un autre type de souci : les bins les plus à gauche sont tellement écrasés que la l'interprétation est malaisée. Cela est solutionné dans l'image [C].

 

[C] Après cette pull request (ERT_IP/analyse.py avec ERT_IP/data/B52_DDN6_TP.ohm):

  • On revient à density=False, de sorte que l'axe vertical signifie à nouveau "nombre de mesures" aussi bien en ERT qu'en IP.
  • Conséquemment, pour ne plus avoir le souci de sur-représentation à droite cité en [A], on mets l'axe des x en log, chose visible
  • Cela rend au passage lisibles les bins à gauche de l'histogramme, en résolution de [C]
  • On voit maintenant distinctement qu'il y a une anomalie résistive sur la plage ~[1.5, 4.5] ohms (ou bien, est-ce une résistance de contact ?)

Bien que c'est des petits bugs, ils ont un impact significatif sur le résultat. Comme on n'a pas le temps de debug en TP (moi compris : j'aurais répondu autrement à la question 3A si j'avais vu çà), je pense que cette pull request sera utile aux suivants. Néanmoins, si vous voulez vérifier le fonctionnement de cette PR avant sa fusion éventuelle, vous pouvez, alternativement à cette page, consulter le dossier ERT_IP de mon fork de votre dépôt sous la branche dev ici voire le cloner et faire git checkout ec0b6d65897fcf6836ea52b316de431889721248 pour le tester (j'aurais peut-être delete la branche dev d'ici-là).

Je marque cette pull request (PR) d'éditable par les propriétaires du dépôt original: n'hésitez pas à traficoter voire revert certains de mes commits dans la liste au bas de cette page en considérait cette PR comme la vôtre. Je pourrais éventuellement aider début juillet si souci git/github/code.

Changelog (les deux modifs importantes sont mises en gras) :

  • ERT_IP/README.md :
    • Lien vers le script analyse.py directement dans le texte
    • Suppression de la version statique de ce code dans le README, afin d'être sûr que les étudiants prendront bien le lien avec la version maintenue à jour
  • ERT_IP/analyse.py :
    • [RÉSOLU] ces bugs causaient un affichage incorrect de l'histogramme des résistances - Voir commit 9d6c066 et suivants :
      • L’interaction entre deux APIs numpy comportait une confusion dans les bases des logarithmes - cfr documentation de numpy.logspace et numpy.log versus numpy.log10
      • Le troncage du dernier décile de données n'était sans doute qu'un dirty fix et est supprimé puisqu'il coupe une partie de l'histogramme
      • L'axe horizontal n'était pas en logarithme alors que les bins l'étaient, causant les difficultés visuelles suscitées
    • Quelques exemples de variables globales pratiques pour la configuration rapide
    • Si exécuté depuis un clone du repo github, lis ERT_IP/data/B52_DDN6_TP.ohm (configurable)
    • L'affichage de l'IP se fait désormais avec le même nombre de 20 bins que précisé pour l'ERT (configurable)
    • L'IP s'indique désormais en V/V et non mV/V vu les données en V/V dans le fichier de TP
    • Formatage plus aérée du code et des commentaires, ajout minime de documentation (notamment, je laisse une trace que density DOIT être False si affichage logarithmique - sans justification afin de ne pas alourdir la lecture) + divers autres refactors d'avantage de l'ordre du beautifying que fonctionnels (détaillés un à un en titres des commits de la liste de cette pull request).
  • ERT_IP/data/B52_DDN6_essai.ohm remplacé par ERT_IP/data/B52_DDN6_TP.ohm : par cohérence avec le fichier fourni sur Teams - il a été vérifié par git diff (cfr 0cf377d) qu'hormis les unités de l'IP (V/V sur Teams au lieu de mV/V), les données sont parfaitement identiques à quelques inversions de signes près
  • ERT_IP/data/B52_DDN6_RecirpocalModel.xlsx : remplacement par la version du fichier sur Teams (par cohérence justifiée dans 7d54513, à l'instar de ERT_IP/data/B52_DDN6_TP.ohm)
  • ERT_IP/HistogramData.py : formatage fidèle à celui d'analyse.py puisqu'il ne s'agissait que d'une adaptation destinée à ERT_IP/data/B52_Gradient7.dat, autrement identique en tout point (cela est mis en évidence par comparaison des fichiers faisant tous deux 61 lignes)

Romadelf added 28 commits June 2, 2025 23:04
(certes, le contenu ne sera lui plus disponible sur la page github)
… logarithmique

Du coup, peut être que ça peut être True pour l'IP. À tester quand le reste du code sera bon.

Cfr https://matplotlib.org/stable/api/_as_gen/matplotlib.pyplot.hist.html
(la doc pandas indique que cette option kwargs est simplement passée à matplotlib)

This reverts commit 9965b83.
…cile, suite au déboguage du commit précédent
Documente aussi que density doit être false selon la doc pandas (qui indique passer cet argument à matplotlib)
(cfr https://matplotlib.org/stable/api/_as_gen/matplotlib.pyplot.hist.html)
Outre quelques changements de signes (par ex. ligne 194), le fichier teams a des valeurs d'IP 1000 fois plus petites.
C'est donc des volts au lieu de milivolts -> label correctement remplacé aussi dans le script
Vérifié par export texte puis git diff qu'il s'agit des mêmes données :

Les seules différences sont les numérotations des électrodes.
La nouvelle version (new) d'un numéro est inférable depuis l'ancienne (old) via la relation `new = (old / 2) + 1`
…t maintenant les mêmes pour une comparaison plus rapide.
@hadrienmichel hadrienmichel requested a review from tomdebouny June 5, 2025 08:10
Repository owner deleted a comment from tomdebouny Jun 5, 2025
@Romadelf
Copy link
Copy Markdown
Author

Romadelf commented Jun 5, 2025

@tomdebouny

Non, ce n'est pas un pull (un externe ne peut faire çà) mais un pull request ("PR"). Le code du github n'est actuellement pas affecté. Il peut éventuellement l'être si elle est acceptée. Comme, je dit dans le message de base, un preview du github modifié est visible sur mon fork.

J'étais développeur avant de reprendre les études et contribues parfois à de l'open source. Le github autorisant les pull request, j'ai supposé que c'était implicitement encouragé. Mes excuses si c'était malvenu (il suffit alors de refuser la PR).

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.

1 participant