Refactor DanceAiClient code #54851

fisher-alice · 2023-11-13T03:28:31Z

This PR updates DanceAiClient.ts for easier readability; there is no change in functionality.
I replaced lists with maps, renamed variables/functions and added/updated comments. In particular, I updated comments so that recent changes are reflected, e.g., using Ada instead of Spacy model values and retrieving the top 3 or bottom 3 options based on ChooseEffectsQuality.

Links

Testing story

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

Tests provide adequate coverage
Privacy and Security impacts have been assessed
Code is well-commented
New features are translatable or updates will not break translations
Relevant documentation has been added or updated
User impact is well-understood and desirable
Pull Request is labeled appropriately
Follow-up work items (including potential tech debt) are tracked and linked

thomasoniii · 2023-11-13T15:04:58Z

apps/src/dance/ai/DanceAiClient.ts

+  const outputWeightsForSelectedEmojis: {[key: string]: number[]} = {};
+  for (const output in cachedWeightsMappings) {
+    outputWeightsForSelectedEmojis[output] = calculateOutputWeightsVector(
+      selectedEmojis,
+      cachedWeightsMappings[output]
+    );
+  }


not blocking at all, but I'd tend to do something like this in a single statement with a reduce:

const outputWeightsForSelectedEmojis: {[key: string]: number[]} = Object.entries(cachedWeightsMappings).reduce( (bucket, [key, cachedMapping]) => ( { ...bucket, [key] : calculateOutputWeightsVector(selectedEmojis, cachedMapping) } ), {} )

(not tested, so there may be typos)

I'd like to keep things as easy to understand for future readers not familiar with context so I think I may keep as is for that reason.

thomasoniii · 2023-11-13T15:05:26Z

apps/src/dance/ai/DanceAiClient.ts

+      quality === ChooseEffectsQuality.GOOD
+        ? options.slice(0, numRandomOptions)
+        : options.slice(-numRandomOptions);
+    allOutputOptions[output] = topOrBottomOptions;


same comment as above about doing this in a reduce vs the external define + for/in.

thomasoniii · 2023-11-13T15:05:59Z

apps/src/dance/ai/DanceAiClient.ts

+    const selectedOutputOption =
+      outputOptions[Math.floor(Math.random() * outputOptions.length)];
+    chosenEffects[output as keyof GeneratedEffect] = selectedOutputOption[1];
+  }


and again, could be a reduce. But doesn't have to be. 😄

thomasoniii

couple of non-blocking stylistic suggestions about using a reduce instead of a for/in, but definitely not blocking.

otherwise, lgtm 🚀

sanchitmalhotra126

Great changes - thanks for doing this! Just had a few comments for further refactoring

sanchitmalhotra126 · 2023-11-13T19:14:59Z

apps/src/dance/ai/DanceAiClient.ts

-
-  // Sort and slice top scoring options, mapped to their output identifiers (e.g. [[0.25, 'squiggles'], ...])
+  // Obtain final output summed weights based on set of three selected emoji inputs
+  const cachedWeightsMappings: {[key: string]: CachedWeightsMapping} = {


nit: for extra type safety, you could even make this {[key in FieldKey]: CachedWeightsMapping}

sanchitmalhotra126 · 2023-11-13T19:16:57Z

apps/src/dance/ai/DanceAiClient.ts

-
-  // Sort and slice top scoring options, mapped to their output identifiers (e.g. [[0.25, 'squiggles'], ...])
+  // Obtain final output summed weights based on set of three selected emoji inputs
+  const cachedWeightsMappings: {[key: string]: CachedWeightsMapping} = {


Also minor - could this be declared outside of this function (alongside CachedPalettes, etc) since it's always the same for every function invocation?

Nice! And removed the individual mappings variables since no longer needed!

sanchitmalhotra126 · 2023-11-13T19:30:53Z

apps/src/dance/ai/DanceAiClient.ts

+  // selectedEmojiAssociations is an array of 3 subarrays. Each subarray contains
+  // the weights ranging from [0, 1] for the selected emojis for a particular output type)


Don't have to, but if we want to get super strict about this, we can enforce these length requirements with TS directly - something like type EmojiAssociations = [number[], number[], number[]]

I tried [number, number, number] but got back the error:
'Type 'number[]' is not assignable to type '[number, number, number]'.
Target requires 3 element(s) but source may have fewer.'
I tried changing type of param emojis to [string, string, string] but error still persisted for some reason. So changed back return type back to number[].

sanchitmalhotra126 · 2023-11-13T19:37:07Z

apps/src/dance/ai/DanceAiClient.ts

+  const allOutputOptions: {[key: string]: [number, string][]} = {};
+  for (const output in outputSummedWeightsForSelectedEmojis) {
+    const weightVector = outputSummedWeightsForSelectedEmojis[output];
+    const options = obtainOptions(weightVector, cachedWeightsMappings[output]);


While we're here - would it make sense to rename this function to something like get/obtainSortedOptions?

Changed to getSortedOptions.

sanchitmalhotra126 · 2023-11-13T19:45:11Z

apps/src/dance/ai/DanceAiClient.ts

  };
-
+  // Select a random option for each output type from the top or bottom scoring options.
+  for (const output in allOutputOptions) {


General comment about the whole function: If I'm reading this correctly, it looks like we have three for loops that iterate over the same options each time. We're also accessing cachedWeightsMappings[output] a couple different times. Could we organize this with just one loop so we just do the full pass for each output type once? Something like

for (const field of [FieldKey.BACKGROUND_EFFECT, FieldKey.FOREGROUND_EFFECT, ...]) { const mapping = cachedWeightsMappings[field]; const weightVector = calculateOutputSummedWeights(emojis, mapping); const options = obtainOptions(weightVector, mapping); // ... calculate top options, randomly select, etc chosenEffects[field] = effect; }

I love this idea. Will do.

sanchitmalhotra126

LGTM!

sanchitmalhotra126 · 2023-11-14T15:11:59Z

apps/src/dance/ai/DanceAiClient.ts

+  for (const field of [
+    FieldKey.BACKGROUND_EFFECT,
+    FieldKey.FOREGROUND_EFFECT,
+    FieldKey.BACKGROUND_PALETTE,
+  ]) {


Oh yeah I guess this could also just be for (const field of Object.keys(chosenEffects) since that's already been declared

Changed to for (const field of Object.keys(chosenEffects) as FieldKey[]) {
thanks!

Ahh interesting that you need to cast with Object.keys - as an alternative, I think you could also use for (const field of Object.values(FieldKey) and I think that shouldn't require a cast (just tried manually). If you prefer, you could also use that below in the helper function that iterates through the same keys.

Nice! Just pushed change.

refactor code

0009bc9

fisher-alice changed the title ~~refactor code~~ Refactor DanceAiClient code Nov 13, 2023

fisher-alice added 3 commits November 13, 2023 03:33

remove print

00e152b

add more comments

da6afb1

add return type

dc114b1

thomasoniii reviewed Nov 13, 2023

View reviewed changes

thomasoniii approved these changes Nov 13, 2023

View reviewed changes

update comments/names

82a7cd1

fisher-alice marked this pull request as ready for review November 13, 2023 16:45

fisher-alice requested a review from a team November 13, 2023 16:45

sanchitmalhotra126 reviewed Nov 13, 2023

View reviewed changes

fisher-alice added 2 commits November 13, 2023 23:28

review feedback

1f16f41

change type due to error

702c4c5

fisher-alice requested a review from sanchitmalhotra126 November 13, 2023 23:35

fisher-alice added 2 commits November 13, 2023 23:41

fix comment return type

d7839ea

update comment

38687f9

sanchitmalhotra126 approved these changes Nov 14, 2023

View reviewed changes

fisher-alice added 2 commits November 14, 2023 15:26

use object.keys

07d1d06

simplify for header

e5be50b

fisher-alice merged commit b9f0293 into staging Nov 15, 2023
2 checks passed

fisher-alice deleted the alice/update-ai-model-client-comments branch November 15, 2023 18:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor DanceAiClient code #54851

Refactor DanceAiClient code #54851

fisher-alice commented Nov 13, 2023 •

edited

thomasoniii Nov 13, 2023

fisher-alice Nov 13, 2023

thomasoniii Nov 13, 2023

thomasoniii Nov 13, 2023

thomasoniii left a comment

sanchitmalhotra126 left a comment

sanchitmalhotra126 Nov 13, 2023

sanchitmalhotra126 Nov 13, 2023

fisher-alice Nov 13, 2023

sanchitmalhotra126 Nov 13, 2023

fisher-alice Nov 13, 2023

sanchitmalhotra126 Nov 13, 2023

fisher-alice Nov 13, 2023

sanchitmalhotra126 Nov 13, 2023 •

edited

fisher-alice Nov 13, 2023

sanchitmalhotra126 left a comment

sanchitmalhotra126 Nov 14, 2023

fisher-alice Nov 14, 2023

sanchitmalhotra126 Nov 14, 2023

fisher-alice Nov 14, 2023

		// selectedEmojiAssociations is an array of 3 subarrays. Each subarray contains
		// the weights ranging from [0, 1] for the selected emojis for a particular output type)

Refactor DanceAiClient code #54851

Refactor DanceAiClient code #54851

Conversation

fisher-alice commented Nov 13, 2023 • edited

Links

Testing story

Deployment strategy

Follow-up work

Privacy

Security

Caching

PR Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasoniii left a comment

Choose a reason for hiding this comment

sanchitmalhotra126 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanchitmalhotra126 Nov 13, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanchitmalhotra126 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fisher-alice commented Nov 13, 2023 •

edited

sanchitmalhotra126 Nov 13, 2023 •

edited