Problème quand lfserie_name est une expression dans reView

[FanchMorvan]

L'application reView ne fonctionne pas correctement quand lfserie_name ou hfserie_name est une expression.

L'appel :

reView(twoStepsBenchmark_object, 
       hfserie_name = "mc.p41_gd10div_2_alim",
       lfserie_name = "a.p3m_faz1_3 + a.p3m_gc10div_3 + a.p3m_gc10e_3 + a.p3m_gc11z_3")

Ce que ça donne :

Cela semble venir de la fonction make_new_bn, et en particulier du parse :

Je ne sais pas si on peut facilement régler ce problème, et s'il est absolument nécessaire de passer par ce assign-parse... On peut bien sûr s'assurer de ne pas avoir d'expressions de notre côté dans les deux arguments en question, mais ils sont assez naturellement présents dans quelques cas (plutôt rare, mais à traiter malgré tout...).

[arnaud-feldmann]

Hello,

Merci de ta remarque pertinente.

La raison d'être de cette construction avec assign parse c'est pour que l'objet twoStepsBenchmark engendré ait un argument call propre, sans bidouiller l'objet post-constructeur (si on s'autorise cela le code perd vite en clarté et en déboggabilité). Deux raisons à cela :

• L'argument call peut être utilisé par ailleurs dans d'autres endroits du package (aussi si c'est modifié il faut faire attention). En particulier des deparse dans reView pour obtenir les noms des séries.
• L'argument call est ce qui est utilisé pour afficher l'objet benchmark en print dans la console, et il faut que ce qui sorte de reView s'affiche proprement à l'utilisateur sous la forme twoStepsBenchmark("nom de l'hfserie","nom de la lfserie", ...). Ce qu'il faut éviter avant tout, c'est d'afficher des calculs internes à reView dans l'appel.

Cela dit, ta demande me semble légitime dans certains cas :

• Si lfserie_name provient de quelque chose d'écrit par l'utilisateur dans reView, ça me semble pas illégitime de bloquer les noms vraiment bizarres. Après tout, cet argument me semble fait pour rendre l'appel propre, pas pour rajouter des cas bizarres.
• Si lfserie_name provient d'un réel appel d'expression, tu as totalement raison, cela ne devrait pas provoquer de bug.

Je n'ai pas trop le temps, mais effectivement le deuxième cas doit être résolu. A voir comment on trouve une solution qui ne triture pas l'objet post-constructeur.

[arnaud-feldmann]

Note @FanchMorvan : il y a un argument cl au constructeur twoStepsBenchmark, il peut sans doute permettre de résoudre ce pb de manière propre, en reconstruisant l'appel twoStepsBenchmark "qui va bien". C'est moins encapsulé, et il faut faire attention à ne pas accepter trop de choses, mais c'est sans doute plus lisible que mon eval substitute assign parse. J'y réfléchis demain.

[FanchMorvan]

Merci pour l'explication, je comprends mieux ce qui est fait et pourquoi on le fait !

Je l'admet, si je devais faire je modifierai l'objet post-constructeur. L'argument call se trouve à deux endroits dans twoStepsBenchmarks, donc il faut le modifier deux fois, et en effet ce n'est pas très joli...

bn_new$call$hfserie <- parse(text = enquote(hfserie_name))[[2]]
bn_new$call$lfserie <- parse(text = enquote(lfserie_name))[[2]]

bn_new$regression$call$hfserie <- parse(text = enquote(hfserie_name))[[2]]
bn_new$regression$call$lfserie <- parse(text = enquote(lfserie_name))[[2]]

La ligne suivante semble permettre de reconstruire l'appel que l'on recherche :

cl <- call("twoStepsBenchmark", hfserie = parse(text = enquote(hfserie_name))[[2]], lfserie = parse(text = enquote(lfserie_name))[[2]], include.differentiation = include.differentiation, include.rho = include.rho, set.coeff = set.coeff, set.const = set.const, start.coeff.calc = start.coeff.calc, end.coeff.calc = end.coeff.calc, start.benchmark = start.benchmark, end.benchmark = end.benchmark, start.domain = start.domain, end.domain = end.domain, outliers = outliers)

La deuxième solution a en effet l'avantage de créer l'appel avant sa construction, et donc d'éviter de le "bidouiller" après, mais je trouve ça un peu moins sécurisé que d'utiliser l'appel généré automatiquement puis de ne modifier que ce que l'on veut modifier...

[arnaud-feldmann]

C'est vrai que, même si seule ta première solution brise le constructeur explicitement, fabriquer son call le brise de facto de toutes manières, puisqu'un de nos successeurs qui modifierait twoStepsBenchmark en pensant avoir modifié au bon endroit se tirerait les cheveux dans les 2 cas en découvrant des call bizarres, incohérents avec son constructeur. Le principe de localité est brisé.

Dans les deux cas, on crée de toutes manières de facto un constructeur tierce.

Pour rester discipliné, peut-être que la meilleure solution est de passer ce constructeur secondaire laveur d'appel en fonction interne dans twoStepsBenchmark.R.

Je t'avouerais que je garde ma préférence pour la deuxième solution, plus verbieuse certes mais plus disciplinée. Un argument call existe et est utilisé notamment pour reUseBenchmark, autant l'utiliser plutôt que créer de nouveaux types d'entorses.

Si tu veux faire une PR, je peux relire à posteriori si tu le souhaites. Mets également un test associé. N'hésite pas à te mettre en ctb dans la description.

[FanchMorvan]

À la réflexion, je ne suis pas sûr de la pertinence de ce que l'on cherche à faire.

Sur un petit exemple :

Quelques remarques :

• déjà, benchmark_old et benchmark sont inversés dans la sortie de reView... c'est assez simple à corriger.
• comme je les comprends, hfserie_name et lfserie_name sont des noms d'affichage dans reView (et il me semble qu'il est du droit de l'utilisateur de vouloir afficher une expression bizarre si cela lui plaît), et ne devraient pas ressortir dans la sortie, au risque de tomber sur le problème que l'on voit ici : évaluer le call du benchmark résulte en une erreur... même si j'ai mis un nom tout à fait correct ici.
• Comme c'est déjà fait, il n'y a pas besoin de mettre les valeurs hfserie_name et lfserie_name dans les arguments call des différents benchmarks pour que l'on ait le bon affichage dans reView, que ce soit pour le titre de l'application ou dans le troisième onglet.

En conclusion, dans get_new_bn (qui est le seul endroit où make_new_bn est utilisé), on ne devrait pas donner comme noms de hfserie et lfserie les valeurs de hfserie_name et lfserie_name, mais les noms observées dans le call de new_bn_external_setter (et qui devraient être les mêmes noms que dans tous les call d'objets twoStespBenchmark manipulés dans reView).

J'espère que j'ai été clair, et je ne sais pas si j'ai bien compris les rôles de hfserie_name et lfserie_name comme tu les envisageais ?

[arnaud-feldmann]

• Pour l'inversion : OMG, une inversion pareille est restée inaperçue si longtemps ? A changer dans une autre issue. Merci beaucoup. Bien prévenir l'utilisateur de ce breaking change avec une instance de :

  warning_news_factory <- function(message) {
  force(message)
  thrown <- FALSE
  function() {
  if (thrown == FALSE) {
    warning(message, call. = FALSE)
  }
  thrown <<- TRUE
  invisible(message)
  }
  }

  Cette fonction devant être planquée dans les utils comme dans la version 1.0.3 ( https://github.com/InseeFr/disaggR/blob/d0eda2e4abf6a12b9e0ccb538f2ac3caa5187ff7/R/utils.R ) et instanciée dans le global du package (cf https://github.com/InseeFr/disaggR/blob/d0eda2e4abf6a12b9e0ccb538f2ac3caa5187ff7/R/in.R ) pour être instanciée au build time et n'avoir qu'un seul warning par session.

• J'ai écrit twoStepsBenchmark avant reView. Quand j'ai écrit reView, mon idée était (comme dans des fonctions d'affichage graphique de rbase) d'utiliser un nom par défaut quand il est évident, mais de laisser l'utilisateur choisir par dessus car parfois, les fonctions de rbase qui mettent des titres par défaut à rallonge m'énervent. Précisément quand c'est une expression d'ailleurs. Je trouve que "ads * x$data + b" en ordonnée est déplaisant. C'est sans-doute pour cela que je n'ai pas envisagé le cas où l'utilisateur veut un nom bizarre, sous forme d'expression. Je n'y ai pas pensé. Mais tu as raison, cela doit être envisagé.
• Dans ma tête, hfserie_name et lfserie_name étaient un peu plus que simplement des affichages dans reView, ils sont aussi ce qui détermine l'affichage de l'objet utilisateur. Honnêtement, je ne l'ai pas fait dans l'optique d'être réévalué

[arnaud-feldmann]

Dans ma tête, le but de l'argument call à la sortie de reView était principalement informatif "tu as pris cette série d'indic, cette série lf, et tu as appliqué ces arguments". Bref pour avoir un print qui veuille dire quelque chose. Mais dans la mesure où je n'ai pas le recul d'usage, libre à vous de faire évoluer la chose de manière qui vous semble plus pertinente.

[FanchMorvan]

Je vois la logique. Dans nos usages, on n'utilise en effet jamais les arguments call directement, je ne sais pas ce qui serait le plus pertinent... Avoir un call qui puisse être réutilisé directement me semble être une plutôt bonne propriété, mais être lisible en est aussi une.

Dans ce cas il me semble que la meilleure solution serait d'ajouter les arguments lfserie_name et hfserie_name à la fonction twoStepsBenchmark_impl, comme cl. Ou alors de créer un autre constructeur comme tu dis, mais cela ne me semble pas apporter grand chose, je ne sais pas quelles pourraient être les bonnes pratiques sur le sujet ?

Et donc de changer le call directement dans twoStepsBenchmark_impl avec les deux lignes suivantes :

if(!is.null(hfserie_name)) cl$hfserie <- parse(text = enquote(hfserie_name))[[2]]
if(!is.null(lfserie_name)) cl$lfserie <- parse(text = enquote(lfserie_name))[[2]]

Il me semble aussi qu'il faudrait harmoniser entre le benchmark_old et le benchmark de la sortie de reView (benchmark reprends les noms lfserie_name, hfserie_name, et pas benchmark_old). Cela passerait par la re-création de bn_old, avec les noms lfserie_name et hfserie_name. Mais c'est plus du détail.

[arnaud-feldmann]

Si tu veux garantir une évaluation juste, il faut encapsuler un environnement comme dans une quosure. Et rajouter que l'évaluation sera correcte dans le contexte de l'environnement encapsulé. Ce serait peut-être plus propre, mais ça complexifierait le package, avec une sorte de NSE, pour pas beaucoup de gain. reView est une application avant d'être une fonction, ce n'est à priori pas appelé par nos utilisateurs dans un processus de prod qui ferait des trucs vraiment tarabiscotés.

Mais fais en tirant partie de l'usage actuel aux comptes trims que je n'ai plus.

[arnaud-feldmann]

Il me semble aussi qu'il faudrait harmoniser entre le benchmark_old et le benchmark de la sortie de reView (benchmark reprends les noms lfserie_name, hfserie_name, et pas benchmark_old). Cela passerait par la re-création de bn_old, avec les noms lfserie_name et hfserie_name. Mais c'est plus du détail.

ça je trouverait ça bizarre. On se permet de modifier le benchmark créé par reView, _old n'est qu'une archive, un rappel. Et y toucher je pense serait un comportement surprenant.

[arnaud-feldmann]

J'ai fait une branche avec une proposition dedans. J'ai unifié avec la correction du bug du sens de l'output. Dorénavant :

• Les noms dans le reViewOuput sont des expressions
• A l'intérieur du module de reView (et de rePort) les noms sont des expressions.
• Les arguments name de rePort et reView peuvent être des expressions ou des character. Si ce sont des character, ils sont convertis en symboles avec une syntaxe modifiée pour être valide dans R (via make.names) en jetant un warning dans le cas où le nom est modifié.

De cette manière, je crois réunir le meilleur des deux mondes :

• Si on ne change rien, c'est qu'on veut à priori garder l'expression, et l'expression est conservée sans l'abimer. Une réévaluation dans un environnement similaire fonctionnera à priori.
• Si on change le nom par un character, c'est qu'on est à priori intéressé par la beauté de la sortie. Il est donc légitime d'écraser l'expression.

Et puis ce qui est accepté est sans doute le plus large éventail que l'on puisse stocker légalement dans une expression. Note : tous les noms convertis depuis des character étant dorénavant passés sous make.names, pour être conformes à la définition du langage R d'un identifier https://cran.r-project.org/doc/manuals/r-release/R-lang.html#Identifiers , les espaces et autres sont dorénavant modifiés (la définition stricte d'un identifier est plus stricte que ce qui est permis par assign, qui accepte les espaces sans pb par exemple).

Comme dit

@ThomasLaurentInsee @FanchMorvan ça vous va ? Je ne sais pas si vous faites lien avec le module interne de reView, mais il faut lui envoyer un objet langage, désormais. Et il faut intervertir le sens de l'output dans le bon nouveau sens. Bonne journée, Arnaud