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

add average frequency tag #1479

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions ext/css/display.css
Original file line number Diff line number Diff line change
Expand Up @@ -1934,6 +1934,15 @@ button.footer-notification-close-button {
:root[data-anki-enabled=false] .action-button[data-action=save-note] {
display: none;
}

:root[data-average-frequency=false] .frequency-group-item:last-child {
display: none;
}

:root[data-average-frequency=true] .frequency-group-item:not(:last-child) {
display: none;
}

Comment on lines +1937 to +1945
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a fragile way of setting this up. It would be better to have specific ids or a class.

:root[data-audio-enabled=false] .action-button[data-action=play-audio] {
display: none;
}
Expand Down
5 changes: 5 additions & 0 deletions ext/data/schemas/options-schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@
"showGuide",
"enableContextMenuScanSelected",
"compactTags",
"averageFrequency",
"glossaryLayoutMode",
"mainDictionary",
"popupTheme",
Expand Down Expand Up @@ -236,6 +237,10 @@
"type": "boolean",
"default": false
},
"averageFrequency": {
"type": "boolean",
"default": false
},
"glossaryLayoutMode": {
"type": "string",
"enum": ["default", "compact"],
Expand Down
41 changes: 40 additions & 1 deletion ext/js/dictionary/dictionary-data-util.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,18 +82,57 @@ export function groupTermFrequencies(dictionaryEntry) {
}

const results = [];

/** @type {import('dictionary').AverageFrequencyListTerm} */
const averages = {};
for (const [dictionary, map2] of map1.entries()) {
const frequencies = [];
const dictionaryAlias = aliasMap.get(dictionary) ?? dictionary;
for (const {term, reading, values} of map2.values()) {
for (let {term, reading, values} of map2.values()) {
Comment on lines -88 to +91
Copy link
Member

Choose a reason for hiding this comment

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

It would be better to keep this const and define new variables when you need to modify stuff.

frequencies.push({
term,
reading,
values: [...values.values()],
});
const valuesArray = [...values.values()];
if (reading === null) { reading = ''; }
Copy link
Member

Choose a reason for hiding this comment

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

This feels like the wrong way to handle this.

let {currentAvg, count} = averages[term]?.[reading] ?? {currentAvg: 1, count: 0};
if (valuesArray[0].frequency === null) { continue; }

currentAvg = (count / (currentAvg)) + (1 / (valuesArray[0].frequency));
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary parentheses here.

currentAvg = (count + 1) / currentAvg;
count += 1;

averages[term] = {
...averages[term],
[reading]: {
currentAvg, count,
},
};
}
results.push({dictionary, frequencies, dictionaryAlias});
}

for (const currentTerm of Object.keys(averages)) {
Copy link
Member

Choose a reason for hiding this comment

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

Should averages really be defined as an object if you need to loop over it here?

const readingArray = Object.keys(averages[currentTerm]);
const nullIndex = readingArray.indexOf('');
if (readingArray.length === 2 && nullIndex >= 0) {
Copy link
Member

Choose a reason for hiding this comment

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

Why is this specifically checking for two readings? What is the point of nullIndex? This feels wrong.

const avg1 = averages[currentTerm][readingArray[0]].currentAvg;
const count1 = averages[currentTerm][readingArray[0]].count;
const avg2 = averages[currentTerm][readingArray[1]].currentAvg;
const count2 = averages[currentTerm][readingArray[1]].count;

const newcount = count1 + count2;
const newavg = newcount / (count1 / avg1 + count2 / avg2);

averages[currentTerm][readingArray[nullIndex === 0 ? 1 : 0]] = {currentAvg: newavg, count: newcount};
delete averages[currentTerm][''];
}
}

const avgFrequencies = Object.keys(averages).flatMap((termName) => Object.keys(averages[termName]).map((readingName) => ({term: termName, reading: readingName, values: [{frequency: Math.round(averages[termName][readingName].currentAvg), displayValue: Math.round(averages[termName][readingName].currentAvg).toString()}]})));
Copy link
Member

Choose a reason for hiding this comment

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

Can this be formatted in a sane way? Maybe splitting out to descriptively named variables or at least adding some newlines in there.

results.push({dictionary: 'Average', frequencies: avgFrequencies, dictionaryAlias: 'Average'});

return results;
}

Expand Down
1 change: 1 addition & 0 deletions ext/js/display/display.js
Original file line number Diff line number Diff line change
Expand Up @@ -1175,6 +1175,7 @@ export class Display extends EventDispatcher {
data.resultOutputMode = `${options.general.resultOutputMode}`;
data.glossaryLayoutMode = `${options.general.glossaryLayoutMode}`;
data.compactTags = `${options.general.compactTags}`;
data.averageFrequency = `${options.general.averageFrequency}`;
data.frequencyDisplayMode = `${options.general.frequencyDisplayMode}`;
data.termDisplayMode = `${options.general.termDisplayMode}`;
data.enableSearchTags = `${options.scanning.enableSearchTags}`;
Expand Down
9 changes: 9 additions & 0 deletions ext/settings.html
Original file line number Diff line number Diff line change
Expand Up @@ -1122,6 +1122,15 @@ <h1>Yomitan Settings</h1>
</div></div>
</div>
</div>
<div class="settings-item"><div class="settings-item-inner">
<div class="settings-item-left">
<div class="settings-item-label">Average frequencies</div>
<div class="settings-item-description">Compress frequency tags into one "Average" freqeuncy tag.</div>
Copy link
Member

Choose a reason for hiding this comment

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

Clarify that this is the harmonic average not a normal average also typo "freqeuncy".

</div>
<div class="settings-item-right">
<label class="toggle"><input type="checkbox" data-setting="general.averageFrequency"><span class="toggle-body"><span class="toggle-track"></span><span class="toggle-knob"></span></span></label>
</div>
</div></div>
</div>

<!-- Popup Position & Size -->
Expand Down
2 changes: 2 additions & 0 deletions test/options-util.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ function createProfileOptionsTestData1() {
popupScaleRelativeToVisualViewport: true,
showGuide: true,
compactTags: false,
averageFrequency: false,
compactGlossaries: false,
mainDictionary: '',
popupTheme: 'default',
Expand Down Expand Up @@ -278,6 +279,7 @@ function createProfileOptionsUpdatedTestData1() {
showGuide: true,
enableContextMenuScanSelected: true,
compactTags: false,
averageFrequency: false,
glossaryLayoutMode: 'default',
mainDictionary: '',
popupTheme: 'light',
Expand Down
24 changes: 24 additions & 0 deletions types/ext/dictionary.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -544,3 +544,27 @@ export type TermSource = {
*/
isPrimary: boolean;
};

/**
* Dictionaries containing the harmonic mean frequency of specific term-reading pairs.
*/
export type AverageFrequencyListTerm = {
/**
* Contains the average frequency of a term, with all its readings.
*/
[term: string]: {
/**
* Contains the average frequency of a reading.
*/
[reading: string]: {
/**
* The current average frequency.
*/
currentAvg: number;
/**
* The number of dictionary frequencies used to compute the average.
*/
count: number;
};
};
};
1 change: 1 addition & 0 deletions types/ext/settings.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ export type GeneralOptions = {
showGuide: boolean;
enableContextMenuScanSelected: boolean;
compactTags: boolean;
averageFrequency: boolean;
glossaryLayoutMode: GlossaryLayoutMode;
mainDictionary: string;
popupTheme: PopupTheme;
Expand Down
Loading