Skip to content

Match credit-array indices against impressions in descending order #278

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Sep 22, 2025
Merged

Match credit-array indices against impressions in descending order #278

merged 2 commits into from
Sep 22, 2025

Conversation

apasel422
Copy link
Collaborator

@apasel422 apasel422 commented Sep 11, 2025 •
edited by pr-preview bot
Loading

This also adds some test coverage for N>1, which revealed a number of bugs in the simulator.

Fixes #277

Preview | Diff

@apasel422 apasel422 force-pushed the n-gt-1 branch 2 times, most recently from 9824fe4 to dcebe57 Compare September 11, 2025 14:40
@apasel422 apasel422 changed the title Add e2e test for multi-touch with even value division Match credit-array indices against impressions in descending order Sep 11, 2025
@apasel422 apasel422 marked this pull request as ready for review September 11, 2025 14:43
@michaelkleber
Copy link

michaelkleber commented Sep 11, 2025 •
edited
Loading

Maybe lastNImpressions should be renamed to something like topNImpressions (or bestN?), to match the new thinking? It's only an internal variable name in the algorithm, so hardly matters, but seems consistent to do so.

@apasel422
Copy link
Collaborator Author

Maybe lastNImpressions should be renamed to something like topNImpressions (or bestN?), to match the new thinking? It's only an internal variable name in the algorithm, so hardly matters, but seems consistent to do so.

Makes sense. We may want to replace all instances in the spec of last-N with this terminology. I like top.

@apasel422
Copy link
Collaborator Author

apasel422 commented Sep 11, 2025 •
edited
Loading

I'll wait to do the renaming until after #276 is merged.

csharrison
csharrison reviewed Sep 11, 2025
View reviewed changes
@apasel422 apasel422 force-pushed the n-gt-1 branch 3 times, most recently from 9c10099 to f0a59ba Compare September 12, 2025 12:05
@apasel422
Copy link
Collaborator Author

@martinthomson Friendly ping for this.

@apasel422 apasel422 mentioned this pull request Sep 17, 2025
Merged
@apasel422
Match credit-array indices against impressions in descending order
a85c4e9 
This makes the credit array resemble an infinite list that is truncated
at the point at which its values become 0.

For example, [.5, 0.25, 0.25] now means that the last 3 impressions will
be matched, and the last one will be given half of the credit, while the
next two will receive one quarter each.
martinthomson
martinthomson approved these changes Sep 22, 2025
View reviewed changes
Copy link
Member

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

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

This is the right way around.

@apasel422 apasel422 merged commit c58af49 into w3c:main Sep 22, 2025
3 checks passed
@apasel422 apasel422 deleted the n-gt-1 branch September 22, 2025 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@csharrison csharrison csharrison approved these changes

@martinthomson martinthomson martinthomson approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Modify the credit to be in reverse order
4 participants
@apasel422 @michaelkleber @martinthomson @csharrison