-
Notifications
You must be signed in to change notification settings - Fork 113
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
base: master
Are you sure you want to change the base?
Changes from all commits
520a3e5
61339f6
d166f3c
d9618c6
6ebbf17
d01e371
37b6431
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 = ''; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()}]}))); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 --> | ||
|
There was a problem hiding this comment.
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.