Comment by Didier Vibert on 2023-05-22 10:26:39:
@msarkis tu peux créer la MR et rajouter le lien stp

la décision de le mettre dans l'API dès maintenant me semble un peu prématuré... ça rejoint la discussion sur l'organisation du paramerterStore c++ vs python et le ticket plus général de vérif de syntaxe/cohérence des paramètres.

Comment by Mira Sarkis on 2023-05-22 11:53:52:
mr: https://gitlab.lam.fr/CPF/cpf-redshift/-/merge_requests/497

Comment by Didier Vibert on 2023-06-06 09:08:25:
code python de l'API valid en ce qui me concerne.

il faut à mon sens modifier aussi le code C++ aux 2 endroits mentionnés dans la description:

• soit on considère au niveau C++ que le check du parameter est fait au niveau python, et plus besoin de s'assurer à nouveau de la cohérence (ie on supprime la L144 dans linemodelfitting.cpp)
• soit on considère que le C++ doit aussi s'assurer de la cohérence et on modifie la L101 de linemodelSolve (rajout d'un & logique avec fftprocessing)

Tout cela nous ramène sur la discussion du check du parameters.json et de l'implémentation python. Je rappelle, dans cette discussion, 2 points qui me paraissent essentiels:
1. on ne veut/doit pas dupliquer le code python et C++
2. actuellement on sait utiliser le code C++ depuis python mais la réciproque est fausse.

Donc

• soit on reste comme ça et l'essentiel de la classe parameters doit être faite en C++ (à minima tout ce dont le C++ a besoin) et le code python doit wrapper ça sans duplication, mais on peut décider que le C++ ne fait aucune vérif de cohérence. Cependant le C++ a malgré tout besoin de méthodes qui combinent certains param entre eux, qu'il faut donc éviter de dupliquer en python.
• soit on regarde avec swig si c'est possible et comment utiliser le code python depuis C++

Il me parait plus simple de rester comme aujourd'hui (ie pas d'utilisation du code python en C++)

Comment by Mira Sarkis on 2023-06-06 14:07:42:
Didier Vibert wrote in #note-5:
> code python de l'API valid en ce qui me concerne.
>
> il faut à mon sens modifier aussi le code C++ aux 2 endroits mentionnés dans la description:
> * soit on considère au niveau C++ que le check du parameter est fait au niveau python, et plus besoin de s'assurer à nouveau de la cohérence (ie on supprime la L144 dans linemodelfitting.cpp)
> * soit on considère que le C++ doit aussi s'assurer de la cohérence et on modifie la L101 de linemodelSolve (rajout d'un & logique avec fftprocessing)

Comme tu dis toujours: "ceinture-bretelles" c'est mieux!

Il faut aussi prendre en compte le cas ou on désactive fftprocessing dans linemodel (@::precomputeContinuumfit) alors que useloglambdasampling et fftprocessing sont à True dans param.json ?
Ce cas est bien équivalent à avoir useloglambdasampling=True et fftprocessing=False, sauf qu'on peut pas vraiment le prévoir au tout début!

> Tout cela nous ramène sur la discussion du check du parameters.json et de l'implémentation python. Je rappelle, dans cette discussion, 2 points qui me paraissent essentiels:
> 1. on ne veut/doit pas dupliquer le code python et C++
> 2. actuellement on sait utiliser le code C++ depuis python mais la réciproque est fausse.
>
> Donc
>
> * soit on reste comme ça et l'essentiel de la classe parameters doit être faite en C++ (à minima tout ce dont le C++ a besoin) et le code python doit wrapper ça sans duplication, mais on peut décider que le C++ ne fait aucune vérif de cohérence. Cependant le C++ a malgré tout besoin de méthodes qui combinent certains param entre eux, qu'il faut donc éviter de dupliquer en python.
> * soit on regarde avec swig si c'est possible et comment utiliser le code python depuis C++
je regarde pour voir si c'est faisable!
>
> Il me parait plus simple de rester comme aujourd'hui (ie pas d'utilisation du code python en C++)

Comment by Didier Vibert on 2023-06-13 09:46:59:
Mira Sarkis wrote in #note-6:
>
> Il faut aussi prendre en compte le cas ou on désactive fftprocessing dans linemodel (@::precomputeContinuumfit) alors que useloglambdasampling et fftprocessing sont à True dans param.json ?
> Ce cas est bien équivalent à avoir useloglambdasampling=True et fftprocessing=False, sauf qu'on peut pas vraiment le prévoir au tout début!
>
on en a parlé lors de la dernière réunion amazed: pas de cas spécifique pour ça: si on a fait du fftprocessing en 1st pass et uselolambdasampling est true on reste avec ça pour la 2nd pass même si le fftprocessing est désactivé par le nombre min de sample (100. qu'il faudrait d'ailleurs ajuster)

Comment by Didier Vibert on 2023-06-13 09:49:34:
basée sur #8048 (à merger après)

Comment by Gaelle Daste on 2023-06-27 12:35:42:
Les tests unitaires associés à la classe Parameters ne passent pas. Déjà dans le init de la classe il faut appeler la méthode check_params après l'instanciation de l'objet parameters

Comment by Didier Vibert on 2023-06-28 13:28:13:
Gaelle Daste wrote in #note-11:
> Les tests unitaires associés à la classe Parameters ne passent pas. Déjà dans le init de la classe il faut appeler la méthode check_params après l'instanciation de l'objet parameters

je ne comprend pas ça... attention en python le @_init_@ n'est pas vraiment un constructeur, l'objet est déjà instancié. Mais c'est une méthode appelée immédiatement après la construction.

Comment by Gaelle Daste on 2023-06-28 14:13:41:
Didier Vibert wrote in #note-12:
> Gaelle Daste wrote in #note-11:
> > Les tests unitaires associés à la classe Parameters ne passent pas. Déjà dans le init de la classe il faut appeler la méthode check_params après l'instanciation de l'objet parameters
>
> je ne comprend pas ça... attention en python le @_init_@ n'est pas vraiment un constructeur, l'objet est déjà instancié. Mais c'est une méthode appelée immédiatement après la construction.

Je corrige pour tout le monde (c'était clair dans ma tête mais c'est pas bien ressorti !) :
Les tests unitaires associés à la classe Parameters ne passent pas. Déjà dans le init de la classe il faut appeler la méthode check_params après l'affectation du dictionnaire à l'attribut parameters

Comment by Gaelle Daste on 2023-06-28 15:18:37:
j'ai fait un fix des tests python qui ne passaient pas dans cette issue avec le check_params, en l'état si elle était feedback avant le départ de Mira, aucune raison de pas la repasser en feedback si les dernières modifs conviennent à tout le monde

Comment by Pierre-yves Chabaud on 2023-06-30 14:29:45:
Merged into @develop@ (@7c27c7ff@)