Comment by Ali Allaoui on 2020-08-13 14:48:53:
Avec cette feature, dans le client LAM le fichier parameters.py, qui contient des paramètres par défaut, devient inutile

Comment by Didier Vibert on 2020-11-07 13:58:32:
ce prétraitement pourrait être une méthode du @CprocessFlowContext@ (ou plutôt du @CParameterStore@)

ce qui permettra, de rendre le membre @CprocessFlowContext:m_ParameterStore@ const (std::shared_ptr<const CParameterStore>) ainsi que le getter CProcessFlowContext::GetParameterStore, à condition de pré-traiter le ParameterStore dans @Init@, avant d'initialiser par copie @m_ParameterStore@.

Note, pour l'instant les seules modifs en écritures effectuées dans le cours du run, consiste en quelques appels à @CDataStore::SetScopedParam@ depuis

• @CLineModelSolve::PopulateParameters@ (forçage de @linemodel.fittingmethod@ et @linemodel.firstpass.fittingmethod@)
• @CMethodDTreeCSolve::Solve@ (forçage de @linemodel.fittingmethod@)
• @CZweiModelSolve::PopulateParameters@ (forçage de @zweimodel.fittingmethod@)

Comment by Ali Allaoui on 2020-11-09 17:54:46:

ce pourrait être aussi des méthodes <pre><code class="cpp">COperator::checkParameters(CParameterStore parameters,std::string scope) </code></pre> que CProcessFlow ou CMethod appellerait sur tous les opérateurs utilisés

Comment by Didier Vibert on 2020-11-10 09:34:02:
Ali ALLAOUI wrote in #note-3:
> ce pourrait être aussi des méthodes [...] que CProcessFlow ou CMethod appellerait sur tous les opérateurs utilisés

yep ! en fait il faut toute la hirerachie , ie des @CMethod::checkParameterss(...)@ qui appellent les @COperator::checkparameters()@ du ou des operateurs utilisés dans la méthode, et CProcessFlow qui appelle les @CMethod::checkParameterss(...)@ des méhthode qu'il utilise.

ce qui conduit à repenser les opérateurs/ méthode en les dérivant chacun d'une classe de base CMethode ou COperator (abstraite éventuellement) contenant les methodes virtuelles

et du coup le contextflow devrait sans doute être un membre (pointeur) des classe CMethod, (et potentiellement des classe COperator aussi ? à voir...)

=> ticket "refactor CMethod/Coperator" ?

je pense aussi qu'il faudrait attaquer la suppression des méthodes obsolètes (actuellement, on perds du temps à les mettre à jour) ainsi que renommage de certaines:
eg
chisquare2solve -> templateSolve
chisquareSolve -> trash
chisquareLogSolve -> templateLogSolve

Comment by Mira Sarkis on 2021-07-15 13:24:19:
Hello, en m'inspirant de https://json-schema.org/, et dans le but de spécifier les paramètres de param.json afin de :
1) valider les noms des params,
2) valider les valeurs données ,
3) valider la complitude des params ainsi que le respect de la hiérarchie
4) et meme générer une documentation complète de tous les params

J'ai écrit un premier schema draft (en pièce-jointe) qui sera une base pour discuter de ma proposition.
En gros les bases du schema sont listées comme suit:

• chaque paramètre d'amazed est une propriété.
• une propriété est consiste en: 1) description, 2) type , 3) constraint, 4) hierarchies
  • description : sert principalement à la documentation des propriétés
  • type : donne le type qui peut etre un type standard, e.g., float, int, bool, etc ou meme un objet.
    • je définis un objet comme une entité englobant plusieurs propriétés. Par exemple, le paramètre @ebmv @est un objet à 3 propriétés.
    • il est possible de définir des objets (e.g., @defModuletype@: @priorObject@) qui sera utilisé comme un nouveau type. Le but içi est d'éviter de dupliquer du texte.
  • constraint : contient une liste de règles qui s'appliquent à la valeur passée à chaque param.
    • j'ai défini des règles "standard" comme par e.g., nonNull, positive, PATH, etc.
    • et aussi des règles correspondant aux valeurs acceptées par chaque paramètre. Ces règles sont ajoutés à la fin du fichier de specification.
  • hierarchies : détermine à quel endroit on pourra placer chacune des propriétés. Dans l'exemple suivant, on voit que le paramètre priors peut appartenir à "tplratio" ou "continuumfit"
    <pre>
    Exemple:
    {
    "priors":{
    "desciption": "priors",
    "type": "object",
    "defModuletype": "priorObject", //definition d'un nouveau type d'objet réutilisable par chacun des elements de @hierarchies@
    "properties":
    Unknown macro: { "betaA"}

    ,
    "hierarchies":["tplratio", "continuumfit"]
    },
    "inValuelistContinuumRemovalMethods":["zero", "Median", "IrregularSamplingMedian","raw"] //définition de contrainte
    }
    </pre>

Comment by Didier Vibert on 2023-06-09 10:04:32:

Résumé de la discussion du 8/06/2023

• implémentation du check parameter 100% python dans l'API
• conséquence: le code C++ fait l’hypothèse que la cohérence est pré-testée, et ne fait plus de check: on supprime donc les checks spécifiques fait en C++ (à identifier dans la classe CParameterStore et ailleurs dans le code). Ne subsiste donc en C++ que le check qu'un param existe lorsqu'on y accède (les getter C++ lèvent une exception)
• on supprime aussi du code C++ les "combinaisons de paramètres" qui ne sont pas des check mais peuvent être assimilés à des paramètres intermédiaires déduits des paramètres initiaux. Le code python devra donc rajouter ces paramètres intermédiaires avec donc les méthodes pour les produire
• Le format reste json
• la classe parameter python devra appeler le checker lors de son instanciation (dans le @_init_@)
• on a parlé de check à base d'un tableau csv. Mais il existe un "json schema":https://json-schema.org/ qui permet de définir une grammaire (en json) pour un document json. @msarkis avait déjà regardé cet aspect (cf #note-8). Il me semble que ça permet de traiter tous les check dont nous avons besoin (range et type des param, paramètre requis conditionnellement à la présence et/ou valeur d'autres, hiérarchie de param...)
  Le fichier schema est sans doute un peu fastidieu à écrire (il peut être scindé en plusieurs sous-ensembles), mais ensuite le checker python existe (il y en a plusieurs https://json-schema.org/implementations.html#validator-python), ainsi que des générateurs de documentation.
• on a décidé de ne pas faire de check des sections "inutiles" (j'ai l'impression que si on utilise json-schema, ce n'est pas possible: le checker vérifiera l'intégralité du contenu, mais ce n'est pas avec un prix supplémentaire car les règles pour les "sections inutiles" doivent être écrites lorsque les sections sont utilisées...)
• encapsulation: aucune méthode C++ de la classe CParameterStore ne devrait être utilisée en dehors de la classe @Parameters@ python, ie le code python api (hormis Parameters.py) et client
  ne doivent pas accéder au CParameterStore.

n'hésitez pas à compléter/corriger (j'ai desassigné cette issue pour l'instant).

Comment by Didier Vibert on 2023-08-23 10:13:27:
@fdufresne voici la réponse à tes questions:

Recap questions restantes sur les paramètres:

1 - Actuellement, ebmv est requis dans les cas où ismfit est enabled quelque part. Ajout d'un warning s'il est présent alors que ismfit enabled nul part ?

c'est la question des param inutiles... que fait-t-on dans les autres cas ??? je ne me souviens plus de ce qu'on avait dit.
Peut-etre coder le check avec un mode plus permissif: en mode strict on raise dès qu'un param inutile est présent, en mode permissif on laisse passer (ou warning ? )

2 - Vérifier qu'on peut supprimer:

• 2.1 - templateCatalog.continuumRemoval.decompScales
• 2.2 - templateCatalog.continuumRemoval.binpath

oui tu peux

3 - Dans quels cas linemeas_dzhalf est-il obligatoire ?

tout le temps (idem pour linemeas_redshiftstep)

4 - Dans quels cas linemeas_redshiftstep est-il obligatoire ?

idem

5 - Peut-on supprimer LineModelSolve.linemodel.extremacutprobathreshold ?

non on garde. Il est actif dans le code. Mais on desactive son utilisation en le mettant à -1. On reste comme ça

6 - Peut-on supprimer LineModelSolve.linemodel.modelpriorzStrength ?

oui, plus de trace dans le code c++

7 - Peut-on supprimer linemodel.pdf.bestzoption ? Si non, dans quels cas est il à spécifier ?

oui, plus de trace dans le code c++

8 - Peut on supprimer templateCatalog.continuumRemoval.binpath et continuumRemoval.binpath ?

oui (idem 2. ?)

9 - Peut-on supprimer templateCatalog.continuumRemoval.decompScales et continuumRemoval.decompScales ?

oui (idem 2. ?)

10 - Dans quels cas object.linemodelsolve.linemodel.continuumfit est-il à préciser ?

lorsque que object.linemodelsolve.linemodel.continuumcomponent = tplfit*
(mais si on applique ça il y a qq petit bug dans le code c++ où les param peuvent être lus alors qu'ils ne sont pas pertinents...
=> ouvir un ticket pour ça: je m'en charge #8231 et je le marque comme bloquant du ticker parameters )

11 - Peut-on supprimer object.LineMeasSolve.linemodel.polynomialdegree ? Si non, dans quels cas est-il à spécifier ?

non, il faut le conserver. Le code le lit, mais par erreur ne s'en sert pas. Il y a un ticket pour réparer ça.

12 - Il semblait qu'il y avait des paramètres manquants dans object.LineMeasSolve. Quels sont ces paramètres manquants ?

?

la liste qui est présente dans le parameters_base.json est incomplète si velocityfit=true cf la variation linemeas_lbfgsb qui introduit les 4 param supplémentaires:
emvelocityfitmin, emvelocityfitmax, absvelocityfitmin, absvelocityfitmax

Comment by Fanny Dufresne on 2023-08-23 14:49:26:
1- Concernant comment gérer les paramètres des paramètres inutiles, je propose:

• Si c'est un paramètre "inconnu" qui dans tous les cas n'est jamais utililsé (ex il y a eu une typo), raise une erreur
• Si c'est un paramètres parfois utile mais dans ce cas précis inutilisé, mettre un warning

12. Ça me paraît étonnantdans le cas de linemodelen plus des emvelocityfitmin max etc on précise également obligatoirement les step, ce qu'on ne fait pas là. Est-ce bien voulu ?

Comment by Didier Vibert on 2023-08-23 15:41:30:
Fanny Dufresne wrote in #note-15:
> 1- Concernant comment gérer les paramètres des paramètres inutiles, je propose:
> - Si c'est un paramètre "inconnu" qui dans tous les cas n'est jamais utilisé (ex il y a eu une typo), raise une erreur
> - Si c'est un paramètre parfois utile mais dans ce cas précis inutilisé, mettre un warning

parfait

> 12. Ça me paraît étonnant dans le cas de linemodelen plus des emvelocityfitmin max etc on précise également obligatoirement les steps, ce qu'on ne fait pas là. Est-ce bien voulu ?

comme discuté oralement, pas de [em|abs]velocityfitstep pour linemeas.
en fait le fitter lbfgsb (le fitter est défini par le param fittingmethod) utilise le param velocityfit (true/false) et les min/max pour fitter la vitesse avec un algo de descente de gradient (c'est cet algo qui détermine le step à chaque pas. En revanche les autres fitter (individual/hybrid/svd...) n'utilisent directement pas ces paramètres. C'est l'opérateur linemodel qui les utilise en faisant une boucle avec un step lors de la seconde passe (dans COperatorLineModel::EstimateSecondPassParameters) donc uniquement dans la méthode linemodelsolve.

pour résumer:

• linemodelSolve: velocityfit(true/false) nécessaire, si true (les step,min,max de [em|abs]velocityfit sont nécessaires sinon inutiles (mais le code les lit sans doute ? ) qq soit la fittingmethod
• linemeasSolve: fittingmethod != lbfgs tout les param velocityfit sont inutiles (le true/false aussi), sinon ils sont nécessaires (mais sans le step)

Comment by Fanny Dufresne on 2023-08-23 16:41:27:
Et une nouelle question !
a. Le champ priors est-il obligatoire dans line_model_continuum_fit ?
b. Dans priors, tous les champs sont-ils obligatoires ?
c. J'avais noté de mettre linemeas_dzhalf en "required", mais il n'est pas présent partout, dans quels cas est-il attendu ?
d. Idem pour linemeas_redshiftstep ?
e. Idem pour enable_reliability

Comment by Didier Vibert on 2023-08-24 12:40:07:
Fanny Dufresne wrote in #note-17:
> Et une nouelle question !
> Le champ priors est-il obligatoire dans line_model_continuum_fit ?
> Dans priors, tous les champs sont-ils obligatoires ?

oui pour l'instant. En fait ils ne sont utiles que lorsque @priors.catalog_dirpath@ correspond à un répertoire existant. Si le repertoire n'est pas trouvé, l'objet @CPriorHelper@ est instancié avec un @mInitFaild@ avec un warning. Typiquement, aujourd'hui pour désactiver l'usage de ces priors, on met @priors.catalog_dirpath: ""@.
il Faudrait rajouter un param bool @enable_priors@. Et en fonction rendre la suite nécessaire ou inutile. avec une exception si l'initialisation se passe mal lorsque enable est True => un peu de dev c++ à faire (mais pas très compliqué: je rajoute un ticket => #8229)

rmq: idem pour les params @tplratio.priors@

Comment by Didier Vibert on 2023-08-24 13:19:41:
Fanny Dufresne wrote in #note-17:

> c. J'avais noté de mettre linemeas_dzhalf en "required", mais il n'est pas présent partout, dans quels cas est-il attendu ?
il devrait ! il est nécessaire dans la section linemeasSolve

> d. Idem pour linemeas_redshiftstep ?
idem

> e. Idem pour enable_reliability
obligatoire pour l'instant. Pour le refactor ultérieur, si on opte pour des bool partout pour activer/désactiver certaines sections ou si la présence d'une section suffit à l'activer.

Comment by Didier Vibert on 2023-09-07 14:06:57:
juste une petite description à modifier (cf dernier commentaire non resolved sur gitlab) , je passe en feedback ensuite

Comment by Fanny Dufresne on 2023-09-07 14:16:41:
C'est corrigé, j'ai ajouté la modif de la description au commit déjà existant "code review corrections"

Comment by Pierre-yves Chabaud on 2023-09-12 09:53:55:
Merged into @develop@ (@f807415a@)