Skip to content
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.

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

Grammar setnames fix #303

Merged
merged 11 commits into from
Mar 6, 2024
Merged

Grammar setnames fix #303

merged 11 commits into from
Mar 6, 2024

Conversation

JakeWags
Copy link
Member

@JakeWags JakeWags commented Mar 5, 2024

Does this PR close any open issues?

Closes #301
Depends #302

Give a longer description of what this PR addresses and why it's needed

Previously, there was a strange discrepancy between the downloaded json data setnames and the alt-text generated setnames. This was due to the fact that the alt-text config generation function was not adjusting the element name field prior to export.

By adding this to the alt text, the output seems to be as expected.

Tested with Simpsons and Movie dataset.

Provide pictures/videos of the behavior before and after these changes (optional)

@elizaan
Please confirm that this is correct:

 This is an UpSet plot that visualizes set intersection. To learn about UpSet plots, visit https://upset.app.
 Dataset Properties

The dataset contains 6 sets, and 44 elements, of which 6 are shown in the plot.
Set Properties

The largest set is Male with 18 elements, followed by School with 6, Duff Fan with 6, Evil with 6, Power Plant with 5, and Blue Hair with 3.
Intersection Properties

The plot is sorted by size. There are 12 non-empty intersections, all of which are shown in the plot. The largest 5 intersections are School, and Male (3), the empty intersection (3), Just Male (3), Duff Fan, Male, and Power Plant (3), and Evil, and Male (2).
Statistical Information

The average intersection size is 2, and the median is 2. The 90th percentile is 3, and the 10th percentile is 1. The largest set, Male, is present in 75.0% of all non-empty intersections. The smallest set, Blue Hair, is present in 16.7% of all non-empty intersections.

image

Are there any additional TODOs before this PR is ready to go?

TODOs:

  • Update playwright tests
  • Fix issue with element names for sets with spaces in the name
  • Update delimiter to ~&~

@JakeWags JakeWags requested review from JackWilb and elizaan March 5, 2024 17:58
Copy link

netlify bot commented Mar 5, 2024

Deploy Preview for upset2 ready!

Name Link
🔨 Latest commit 5637779
🔍 Latest deploy log https://app.netlify.com/sites/upset2/deploys/65e8a4e5b771fe0008066b5b
😎 Deploy Preview https://deploy-preview-303--upset2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@JakeWags JakeWags force-pushed the grammar-setnames-fix branch from 80c38b2 to 7067298 Compare March 5, 2024 22:39
const splitElName = r['elementName'].split(' ');
const splitElName = r['elementName'].split('~&~');

let elName = splitElName.join(', ');
Copy link
Member

Choose a reason for hiding this comment

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

Should this be

Suggested change
let elName = splitElName.join(', ');
let elName = splitElName.join(' & ');


if (splitElName.length > 1) {
const lastWord = splitElName.pop();
const elName = splitElName.join(', ');
r['elementName'] = `${elName}, and ${lastWord}`;
// elName = `${elName}, and ${lastWord}`;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// elName = `${elName}, and ${lastWord}`;

} else if (r.type === 'Aggregate') {
const r2 = r as Aggregate;
if (r2.aggregateBy === 'Overlaps') {
elName = elName.split(' - ').join(' & '); // overlaps look like "Adventure - Action", so replace the hyphen with " & "
Copy link
Member

Choose a reason for hiding this comment

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

Is this - coming from somewhere else?

@JakeWags JakeWags merged commit 706e29e into main Mar 6, 2024
7 checks passed
@JakeWags JakeWags deleted the grammar-setnames-fix branch March 6, 2024 18:04
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.

Fix grammar set name/element name errors
3 participants