[App Search] Relevance Tuning logic listeners #89461
Conversation
JasonStoltz
added
Feature:Plugins
release_note:skip
…h/components/relevance_tuning/relevance_tuning_logic.test.ts Co-authored-by: Constance <constancecchen@users.noreply.github.com>
| ...searchFields, | ||
| [name]: { | ||
| ...searchFields[name], | ||
| weight: parseFloat(weight.toFixed(1)), |
There was a problem hiding this comment.
Sorry, I think our discussion about whether we allow numbers vs strings of numbers has me confused. Shouldn't we do parseFloat first and then toFixed in case a '2' is being passed?
| weight: parseFloat(weight.toFixed(1)), | |
| weight: parseFloat(weight).toFixed(1), |
There was a problem hiding this comment.
I ended up reverting this change. toFixed returns a string which is why I have to call parseFloat. The string representation may actually not be parseable back to a float in certain cases. I think the previous implementation with Math.round is the appropriate solution.
There was a problem hiding this comment.
Weight is actually a number, just for reference, it can never be a string.
There was a problem hiding this comment.
Ohh gotcha gotcha, super interesting! If we're doing this in multiple places it might be nice to pull it out to a util so we can clearly explain in a single comment why we're doing it this way & so we don't forget and used toFixed later on in a different place
...s/enterprise_search/public/applications/app_search/components/relevance_tuning/utils.test.ts
Outdated
Show resolved
Hide resolved
...s/enterprise_search/public/applications/app_search/components/relevance_tuning/utils.test.ts
Outdated
Show resolved
Hide resolved
|
Just a comment clarifying |
This reverts commit 738c18e.
…h/components/relevance_tuning/utils.test.ts Co-authored-by: Constance <constancecchen@users.noreply.github.com>
|
Great, thanks for the awesome review. I really do appreciate 👍 |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
* master: (44 commits) [APM] Add experimental support for Data Streams (elastic#89650) [Search Session] Control "Kibana / Search Sessions" management section by privileges (elastic#90818) [Lens] Median as default function (elastic#90952) Implement custom global header banner (elastic#87438) [Fleet] Reduce permissions. (elastic#90302) Update dependency @elastic/charts to v24.5.1 (elastic#89822) [Create index pattern] Can't create single character index without wildcard (elastic#90919) [ts/build_ts_refs] add support for --clean flag (elastic#91060) Don't clean when running e2e tests (elastic#91057) Fixes track_total_hits in the body not having an effect when using search strategy (elastic#91068) [Security Solution][Detections] Adds list plugin Saved Objects to Security feature privilege (elastic#90895) Removing the code plugin entirely for 8.0 (elastic#77940) chore(NA): move the instruction to remove yarn global bazelisk package into the first place on install bazel tools (elastic#91026) [jest/ci] remove max-old-space-size override to use 4gb default (elastic#91020) [Fleet] Restrict integration changes for managed policies (elastic#90675) [CI] Fix auto-backport condditions so that it doesn't trigger for other labels (elastic#91042) [DOCS] Uses variable to refer to query profiler (elastic#90976) [App Search] Relevance Tuning logic listeners (elastic#89461) [Metrics UI] Fix saving/loading saved views from URL (elastic#90216) Limit cardinality of transaction.name (elastic#90955) ...
Summary
This is Part 3 of Relevance Tuning.
I don't know if it will really be helpful to review this PR commit by commit or not, I was not diligent with these commits, and later commits definitely thrash prior ones. So it's up to you.
To move fast and reduce friction in the migration, this code is almost an exact replica of what was in ent-search. I have not changed the logic in any significant way other than to match the current patterns of our code. Please consider reviewing this as just that, a migration. There is much opportunity for refactor, but I don't think we need to take that on right now.
Checklist
Delete any items that are not applicable to this PR.
For maintainers