feat(model): Add model values to represent a numeric value computed from a model - #185
feat(model): Add model values to represent a numeric value computed from a model#185indyteo wants to merge 6 commits into
Conversation
6f3123a to
67562b9
Compare
[Maven Build Status]📑 Commit: 📦 Download artifact: Generator.jar |
pyrollo
left a comment
There was a problem hiding this comment.
Quelques commentaires, plutôt sur le fond.
En fait, j'ai l'impression qu'on peut pousser le truc beaucoup plus loin. Evidemment, on ne va pas avoir le temps tout de suite.
En revanche, ça serait pas mal qu'on se cale un moment pour brainstormer là dessus, histoire de voir ce qui est réaliste à moyen terme et de faire en sorte d'aller doucement vers ça sans se bloquer.
En gros, savoir où on s'arrêtera, où on s'arrête uajourd'hui et savoir à quoi ça pourrait ressembler pour être sûr d'aller dans le bon sens.
| ### Metadata | ||
|
|
||
| The most natural way to provide a value from a model is by reading a metadata from that model. | ||
| It is absent if the model does not have this metadata, or if its value is not a number. | ||
| There are two ways to do it: one is by simply providing the name of the metadata and the other is by using the field `metadata`. | ||
|
|
||
| Example: | ||
| ```yaml | ||
| value: min_height | ||
| otherValue: | ||
| metadata: max_height | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Ça a l'air bien pratique mais j'ai l'impression que ça va nous coincer plus tard si on veut des ModelValues de type chaine, et si on veut piocher les model values dans autre chose (variable globales ? ...).
Je me demande si, même si c'est plus lourd, il ne faudrait pas restreindre à la notation metadata:
There was a problem hiding this comment.
J'ai un peu copié les heightmaps "sans réfléchir" j'avoue, en me disant qu'on se poserait les questions en temps voulu (questions qui seront les mêmes sur les deux sujets je veux dire). Ça me va de retirer le raccourci, mais en attandant (tant qu'il dérange pas), on pourrait aussi bien le laisser à mon sens (j'ai pas l'impression que ça soit ça qui nous empêche de changer par la suite, en tout cas, on n'a aucun contrat de "rétrocompatibilité" de paramétrage (en tout cas pour l'instant, si on veut en instaurer un, pourquoi pas, mais c'est un chantier à part entière))
De toute manière, je suis toujours convaincu que sur ce sujet, "à terme", ça serait bénéfique d'avoir une syntaxe alternative à la forme canonique YAML (en plus, pas à la place, bien sûr), et qui soit bien plus "user-friendly" pour écrire des expressions (genre 2 * (width - 1), max(height, 5)...). Mais c'est pas forcément dans le générateur qu'elle devrait être, ça pourrait se trouver éventuellement à un niveau supérieur. Et si on décide de faire ça, on pourra choisir un symbole comme $ par exemple pour désigner les variables issues des métadonnées. Ce qui ré-introduirait indirectement une forme raccourci value: $height qui ne coincerait pas pour autre chose
There was a problem hiding this comment.
En fait ce qui m'inquiète c'est qu'on va commencer a avoir du paramétrage un peu plus définitif. Donc les modifications non rétrocompatibles vont devenir un peu plus pesantes.
There was a problem hiding this comment.
A tout hasard, j'ai vite fait recherché "express formula in yaml" et je tombe là dessus:
https://learn.microsoft.com/en-us/power-platform/power-fx/yaml-formula-grammar
Je me suis arrêté à It's consistent with Excel...
| - b | ||
| ``` | ||
|
|
||
| ### Fallback |
There was a problem hiding this comment.
Je ne suis pas fan de ce terme qui me parait un peu technique. J'imagine un truc du genre firstOf: ou either: (il y a probablement mieux).
There was a problem hiding this comment.
On avait évoqué la question dans la PR FDS en proposant le terme coalesce. Certes il parlerait + aux ingénieurs de base de données, mais il reste toujours technique 😅
J'ai l'impression que le terme fallback est certes un peu technique, mais reste assez répandu pour être compréhensible par les gens qui ont le niveau suffisant pour avoir besoin de s'en servir. Là où proposer un "nouveau" terme risque de créer du doute chez les sachants, et rester un "nouveau concept" pour les autres (qu'ils découvrent simplement avec un autre nom).
Mais sinon, d'autres propositions pourraient être firstNonAbsent:, ou quelque chose basé sur "alternative" / "or else"
67562b9 to
46d9c2e
Compare
pyrollo
left a comment
There was a problem hiding this comment.
Revue partielle, je continuerai mercredi.
Je pense qu'il faut supprimer le raccourci "XXX" pour prendre la valeur d'une métadata et ne garder que "metadata: XXX" (on sait déjà que ça va nous obliger à revoir le paramétrage plus tard).
Autre chose : les values sont dans models/values mais on sait aussi déjà que ça va se généraliser. Peut être peuvent-elles déjà être placées dans generator/values.
C'est dommage que tous les opérateurs ne puissent rentrer dans Binary/Unary operator.
| /** | ||
| * Model value to return for matching models (required). | ||
| */ | ||
| @JsonSetter(nulls = Nulls.FAIL) |
There was a problem hiding this comment.
L'éternel débat:
Rien n'empêche, techniquement, de faire un if...else, c'est juste les mots then et else qui nous incitent à l'interdire. Ce qui est dommage. Du coup on est obligé d'ajourer un not techniquement évitable.
Possibilités: changer les noms (true/false au lieu de then/else), ajouter une forme unless, ou bien, peut être encore mieux, juste rendre then optionnel sans forcément inciter à l'utiliser.
| Optional<Double> leftValue = leftOperand.get(model); | ||
| Optional<Double> rightValue = rightOperand.get(model); | ||
| if (leftValue.isEmpty() || rightValue.isEmpty()) | ||
| return Optional.empty(); |
There was a problem hiding this comment.
| Optional<Double> leftValue = leftOperand.get(model); | |
| Optional<Double> rightValue = rightOperand.get(model); | |
| if (leftValue.isEmpty() || rightValue.isEmpty()) | |
| return Optional.empty(); | |
| Optional<Double> leftValue = leftOperand.get(model); | |
| if (leftValue.isEmpty()) | |
| return Optional.empty(); | |
| Optional<Double> rightValue = rightOperand.get(model); | |
| if (rightValue.isEmpty()) | |
| return Optional.empty(); |
Eventuellement, ça évite une évaluation de trop
pyrollo
left a comment
There was a problem hiding this comment.
J'ai fini la partie main, reste les tests.
J'ai clos un certain nombre de commentaires mais il reste des trucs à revoir.
| value: height | ||
| greaterThan: 0 |
There was a problem hiding this comment.
En voyant ça me vient une autre idée de syntaxe.
Tout d'abord, je pense vraiment qu'il ne faut pas faire ce raccourci pour les metadata. Donc ça donne:
value:
metadata: height
greaterThan: 0Du coup le "value" fait un peu de trop vu qu'au final on n'aura que des values pour les opérateurs equals/greater/lower.
Ça pourrait être:
is:
metadata: height
greaterThan: 0| @Override | ||
| public Optional<Double> get(Model model) { | ||
| if (model.getMetadata(name) instanceof Number number) | ||
| return Optional.of(number.doubleValue()); |
There was a problem hiding this comment.
Au final, on va surement avoir un "parse" ici quand on aura les values typées. Ce sera un truc du genre:
metadata: XXX
as: integer / text / decimal / booleanMais déjà, on pourrait être aussi permissif que le ValueParser et tenter un Double.valueOf(value.toString())); si la classe n'est pas number.
Ça éviterait d'avoir recours à un postprocesseur parse rien que pour ça.
| } | ||
|
|
||
| @Override | ||
| default double applyAsDouble(Model value) { |
There was a problem hiding this comment.
L'implémentation de l'interface ToDoubleFunction n'est jamais utilisée (sauf dans le test de applyAsDouble).
Je ne vois pas de cas d'usagre où on ferait applyAsDouble plutôt que de tester l'absence (éventuellement avec orElseThrow).
Du coup je n'implémenterais pas cette interface du tout (en plus ça ne passera pas le typage futur).
| case VALUE_NUMBER_INT, VALUE_NUMBER_FLOAT -> new FixedValueParams(parser.getDoubleValue()); | ||
| case VALUE_STRING -> { | ||
| String str = parser.getString(); | ||
| yield str.equals("absent") ? new AbsentValueParams() : new MetadataValueParams(str); |
There was a problem hiding this comment.
Ça aussi ça va poser problème pour le typage. On ne pourra pas indiquer de valeur chaine contenant "absent".
Alors, je ne sais pas quoi faire en revanche. Déjà, dans quel cas a-t-on besoin de créer une valeur absente ? Peut être peut-on faire sans.
Peut être que l'absence de valeur peut se noter sans rien indiquer ("value: " tout court)
Je ne sais pas ce que Jackson renvoie comme type et s'il fait la différence entre:
value:et
value: ""There was a problem hiding this comment.
Je viens de découvrir le mot clé null en yaml:
value: nullvaleur vide
value: "null"chaine "null"
Donc "absent" exite déjà "out of the box" en quelque sorte, c'est null
| private final String metadata; | ||
| private final ModelValue value; | ||
| private final boolean abortIfValueIsAbsent; | ||
| private final boolean doNotOverwriteExistingMetadata; |
There was a problem hiding this comment.
Un peu spécial la négation dans le nom d'un booléen. J'aurais proposé overwriteExsitingMetadata mais je vois que dans le paramétrage il est tourné dans l'autre sens et que ça peut être trompeur de faire la négation. Donc peut être, comme dans le paramétrage keepExistingMetadata ?
| if (cls.isInstance(model)) { | ||
| try { | ||
| run(cls.cast(model), tile); | ||
| } catch (IgnorableException ignored) {} |
There was a problem hiding this comment.
Je ne me souviens plus de ce qu'on avait décidé pour cette exception (ni même si on avait décidé quelque chose). Mais j'ai l'impression qu'il faudrait pouvoir indiquer si on veut ou pas s'arrêter sur les IgnorableExceptions.
Changes
This PR brings model values, which are operators to compute numerical values from models, and integrate them into the codebase.
The main commit is based on the PR #130 from the FDS2025 code-sprint.
Feature description
This PR contains a total of 6 commits each with a relatively well defined scope. I strongly suggest reviewing them separately, in the order described below.
feat(model): Add model values to represent a numeric value computed from a model
The first commit introduces model values, with functional classes, parameters, documentation and unit tests. Functionally, a model value is a function accepting a model and producing an optional double value. It may or may not not use the model, and may or may not rely on other model values. Not every value exists for every model: it may return an empty optional, signaling an "absent" value. It forces every model values to explicitly encode their absent value policy, while still being concise.
Currently, model values only supports numeric values. In the future, this limitation might get removed to allow any type of value. It raises questions about current operators (how to compute the product of strings?), and there will probably be some joint work with heightmaps (that could become datamaps): it will then be the good time to refactor both heightmaps and model values to share their operators.
See
docs/usage/parameters/ModelValues.mdfor full parameters documentation.feat(tasks): Improve existing tasks to accept model values
The second commit improves the existing task able to fill between a heightmap and a metadata value to make it accept a model value instead. Parameters, documentation and unit tests are updated accordingly.
The next two commits are where things begin to go off-road. They both are motivated by their following one, so let's unwrap it starting with the fifth commit.
feat(filters): Improve model filters using model values
This fifth commit integrates model values into model filters. The two concepts are highly intertwined by nature, both taking a model to produce a well-defined result (boolean or numerical value). A new model filter now operates on a model value:
ModelFilterOnValue. It is heavily inspired by its equivalent for metadata,ModelFilterOnMetadataValue.From this functional filter are derived several filter parameters, including "has", "equals" and existing "lowerThan/greaterThan". Existing "has" and "equals" for metadata remains necessary when working with non-numeric values until model values are able to represent them.
However, model values parameters take the generation object as context for their creation, to allow implementations needing special values to exists. This is the case for the random model value needing the generation seed. We might also have special model values referring to generation constants such as the scale (not sure). To have model values in model filters, the latter thus also needs the generation context to be created. This cascades to model selection and post-processors params, leading to the fourth commit.
refactor(params): Pass generation context to params create method
This fourth commit refactors parameters creation to add the generation context object to the
createmethods of all model filters, selection and post-processors parameters.While we originally voted that out a the early stage of the project, I invite us to reconsider it through this PR, especially now that we are really close to upgrading to Java 25, which will bring scoped values, which we already decided to adopt, crossing the main barrier of "unexpected/shadowed dependency" we believed in before. While it was a good idea to try to explicitly limit which object each parameters had to create itself, is now becomes a limitation with our parameters-driven philosophy. To ensure maximum flexibility, each object must have what it needs at his disposal, without needed to add that specific requirement to all objects of its kind. Because the generation object holds all the context usually needed, it seems like the most logical thing to have.
Later on, we might decide to remove this parameter and instead provide its value using a scoped value, to reduce boilerplate code. Especially for unit tests, which is the reason of the third commit.
test(generation): Add TestingGeneration class to simplify tests
This third commit aims at simplifying unit tests when a generation object is needed. A new
TestingGenerationclass is introduced, with an easy-to-(not-)use instanceTestingGeneration.UNUSED.Several tests benefit from this new class centralizing all the arguments of its lengthy constructor. This also simplifies future updates of that constructor, by having a centralized location where it is used in unit tests.
feat(post-processor): Add post-processor to set a metadata value from a model value
The sixth and last commit adds a new post-processor, inspired by the "copy" post-processor, but instead defining a model value into a metadata. For the same reason as cited above, it does not replace it completely, until model values are no longer numerical-only.
This new "set" post-processor can be used both for optimization purpose (pre-compute a model value once for each model and use it multiple times later through its metadata), and for dynamic default numerical value (those value is computed from other metadata values, when present).
Showcase
Many examples can be found in the updated documentations files, especially
ModelValues.mdandModelSelection.md.An upcoming usage will be to compute the height of OSM buildings from the number of floors tags, which could translate to something like:
Note that in this example, the vertical scale is expressed as a YAML reference to an hypothetical anchor that would be placed on the actual scale parameter definition. This would need to adjust our helper script to concatenate the place parameters before the process parameters, which should be acceptable. Another option would be to introduce a special model value, that would be a kind of "generation constant" for the vertical scale. Both options sounds possible, although I personally prefer the first one.
Reason
The great flexibility behind readable heightmaps was missing on metadata values. This concept was approached twice, by 2 different developers, leading to surprisingly similar results! Once on the
pyr/rdrvaluesbranch, and especially this commit e61c2e0 from December 5, 2024, and a second time on thefds/mainbranch, and especially this commit 30bb861 from September 22, 2025.TODOs
Probable future works include:
Self-checks
/docsfolder has been updatedexamples/work the same (or have been adapted if subject to changes in this PR)