Skip to content

25868: Renames internal paramaters for consistency, adds . to all internally computed features, MAJOR - #920

Open
howsoRes wants to merge 6 commits into
mainfrom
25868-consistency-renames
Open

25868: Renames internal paramaters for consistency, adds . to all internally computed features, MAJOR#920
howsoRes wants to merge 6 commits into
mainfrom
25868-consistency-renames

Conversation

@howsoRes

Copy link
Copy Markdown
Member
  1. renames:
    !hyperparameterParamPaths to !dataParametersPaths
    !defaultHyperparameters to !defaultDataParametersMap
    !hyperparameterMetadataMap to !hyperparameterMetadataMap

  2. all other internal variables or parameters referencing 'hyperparameters' to 'data parameters'

  3. internal data parameter 'featureMdaMap' is renamed to 'featureProbabilitiesMap'

  4. adds a leading '.' to conviction-related auto-computed features such as '.distance_contribution' and '.similarity_conviction' for flows where user does not manually specify a feature name

@howsoRes
howsoRes requested review from a team as code owners August 11, 2026 19:36

@howso-mario howso-mario left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The broad dataset-parameter renames, 119.0.0 release migration, and dot-prefixed default feature creation pass the focused tests I ran. I’m requesting changes for three concrete correctness issues: details.amlg now looks up dot-prefixed keys in a map that remains keyed by undotted computation names; the saved-state migration does not rename featureMdaMap inside the default parameter map; and react_aggregate introduces an unused, misspelled public parameter. I’m treating the other public renames as intentional MAJOR-version changes rather than requiring compatibility aliases. CI is also currently blocked by the repeated Linux aggregate-goal test failure.

Comment thread module/details.amlg
can_use_computed_contribution_values
(contains_index (get !computedFeaturesMap "computed_map") "similarity_conviction")
(!= .null (retrieve_from_entity existing_case_id (get !computedFeaturesMap ["computed_map" "similarity_conviction"]) ))
(contains_index (get !computedFeaturesMap "computed_map") ".similarity_conviction")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

!computedFeaturesMap["computed_map"] is keyed by undotted computation names, with dot-prefixed feature names stored as its values. A read-only runtime reproduction after react_into_features produced {similarity_conviction: ".similarity_conviction", residual_contribution: ".residual_contribution"}. Therefore this new lookup for ".similarity_conviction" always misses; the equivalent changes at lines 511 and 550 also miss for residual and distance contribution. The optimized existing-case path consequently recomputes these metrics instead of retrieving their stored feature values, potentially changing results and adding substantial query cost. Please retain the undotted map keys while allowing their mapped feature names to be dot-prefixed, and add a regression test covering details for an existing case after react_into_features.

(assign_to_entities (assoc

!dataParametersPaths (get import_metadata_map "!hyperparameterParamPaths")
!defaultDataParametersMap (get import_metadata_map "!defaultHyperparameters")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The migration copies !defaultHyperparameters directly into !defaultDataParametersMap, but only rewrites nested featureMdaMap keys in !hyperparameterMetadataMap. All new default-map consumers read featureProbabilitiesMap, so an upgraded trainee whose default map contains the old key can silently lose those probabilities and fall back to other weights. Please apply the same recursive key migration to the default map and extend ut_h_migration.amlg to assert both migrated maps contain featureProbabilitiesMap and no featureMdaMap.

; specified robust or non-robust type.
#{type "boolean"}
robust_hyperparameters .null
robust_dataparamseters .null

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

robust_dataparamseters appears misspelled. Because this is a declared public endpoint parameter, the typo becomes part of validation/generated schemas and callers using the natural spelling will be rejected. It is also not referenced anywhere after declaration, so the accepted parameter currently has no effect. Please choose the canonical spelling—consistent with the other snake-case dataset-parameter names—wire it into the intended robust-selection behavior or remove it, and add a focused endpoint test.

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.

3 participants