Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkg: upgrade publicodes to 1.0.0 (NGC-454) #34

Closed
wants to merge 17 commits into from
Closed

Conversation

Clemog
Copy link
Contributor

@Clemog Clemog commented Jan 2, 2024

Important

Pour l'instant toutes les règles dont un de leurs parents contient un mécanisme contexte ne sont pas optimisées. On passe donc de 962 règles à 1147 dans la version optimisée.
Des améliorations pourraient être fait pour optimiser les règles enfants qui le peuvent mais pour l'instant je préfère opter pour la solution simple et qui assure que les résultats du calcul sont fiables (cf. le commentaire dans constantFolding.ts).

@Clemog
Copy link
Contributor Author

Clemog commented Jan 2, 2024

@EmileRolley, j'ai l'impression qu'il y a un peu de taf au niveau de l'optim, je crois que l'AST a changé non?

@Clemog Clemog changed the title pkg: upgrade publicodes to 1.0.0-rc.5 pkg: upgrade publicodes to 1.0.0-rc.5 (NGC-454) Jan 2, 2024
@Clemog
Copy link
Contributor Author

Clemog commented Jan 3, 2024

Je vois qu'il y un souci avec le fait qu'on ait plus accès aux variables traversées dans l'évaluation..
EDIT: c'est en fait une option désormais donc je pense que c'est facilement corrigeable pour le moment :)

@Clemog
Copy link
Contributor Author

Clemog commented Jan 5, 2024

Note : L'import ne fonctionne plus car nom a disparu des mécanismes (cf publicodes/publicodes#420) -> J'ai corrigé normalemnt

Clemog added 6 commits January 5, 2024 18:34
@EmileRolley j'ai tenté un truc mais le souci c'est qu'on n'a plus dans la règle qu'on ne doit pas optimiser l'info du recalcul/context mais seulement via le parent, règle par laquelle on passe "trop" tard
J'ai raconté n'importe quoi dans mon dernier commit ! 😶
Pas sur que ce soit utile mais j'ai toujours une erreur avec l'optim alors que tous les tests passent ici :/
@Clemog
Copy link
Contributor Author

Clemog commented Jan 5, 2024

J'ai essayé d'avancer au mieux !

Je penses qu'il y a un petit soucis avec les variables traversées, j'ai parfois cette erreur :
image

on en rediscute, j'ai l'impression que l'optim fonctionne pas encore sur le modèle NGC avec publiocdes v1 :)

@EmileRolley EmileRolley force-pushed the publicodes-migration branch 2 times, most recently from b006a23 to b92a793 Compare January 9, 2024 14:30
@EmileRolley EmileRolley changed the title pkg: upgrade publicodes to 1.0.0-rc.5 (NGC-454) pkg: upgrade publicodes to 1.0.0 (NGC-454) Jan 9, 2024
@Clemog
Copy link
Contributor Author

Clemog commented Jan 10, 2024

Pour l'instant toutes les règles dont un de leurs parents contient un mécanisme contexte ne sont pas optimisées. On passe donc de 962 règles à 1147 dans la version optimisée.
Des améliorations pourraient être fait pour optimiser les règles enfants qui le peuvent mais pour l'instant je préfère opter pour la solution simple et qui assure que les résultats du calcul sont fiables (cf. le commentaire dans constantFolding.ts).

C'est pas à cause de la disparition de recalculNode dans l'AST ? On peut pas faire en sorte que ça reste ?

@EmileRolley
Copy link
Collaborator

Pour l'instant toutes les règles dont un de leurs parents contient un mécanisme contexte ne sont pas optimisées. On passe donc de 962 règles à 1147 dans la version optimisée.
Des améliorations pourraient être fait pour optimiser les règles enfants qui le peuvent mais pour l'instant je préfère opter pour la solution simple et qui assure que les résultats du calcul sont fiables (cf. le commentaire dans constantFolding.ts).

C'est pas à cause de la disparition de recalculNode dans l'AST ? On peut pas faire en sorte que ça reste ?

Ce n'est pas du à la migration de publicodes. C'est juste que j'étais partis du principe que le recalcule (maintenant contexte) impactait une seule règle. C'est le cas dans le modèle de NGC où les contexte sont uniquement utilisés avec un valeur: <rule_name>. Or, cela n'est pas correct :

  1. le nouveau contexte s'applique à toutes les sous-étapes (sous-règles) nécessaires au calcul de la règle référencée avec le mécanisme valeur
  2. on peut également appliquer un nouveau contexte à une formule plus complexe impliquant plusieurs règles.

Le fait que l'on n'ait actuellement pas eu de problème avec l'optim revient d'un jeu de circonstances propres au modèle actuel de NGC mais n'est pas fiable pour un modèle arbitraire.

@EmileRolley
Copy link
Collaborator

closed in favor of #35

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.

2 participants