add average frequency tag #1479
Conversation
CodSpeed Performance ReportMerging #1479 will not alter performanceComparing Summary
|
|
this is ready for merge, please review |
|
View Playwright Report (note: open the "playwright-report" artifact) |
|
fixed issues with unit tests, now ready to merge |
There was a problem hiding this comment.
Add a settings update for this here: https://github.com/yomidevs/yomitan/blob/master/ext/js/data/options-util.js
And bump the version here: https://github.com/yomidevs/yomitan/blob/master/test/options-util.test.js
| <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.
Clarify that this is the harmonic average not a normal average also typo "freqeuncy".
| } | ||
| } | ||
|
|
||
| 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.
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, frequencies, dictionaryAlias}); | ||
| } | ||
|
|
||
| for (const currentTerm of Object.keys(averages)) { |
There was a problem hiding this comment.
Should averages really be defined as an object if you need to loop over it here?
| for (const currentTerm of Object.keys(averages)) { | ||
| const readingArray = Object.keys(averages[currentTerm]); | ||
| const nullIndex = readingArray.indexOf(''); | ||
| if (readingArray.length === 2 && nullIndex >= 0) { |
There was a problem hiding this comment.
Why is this specifically checking for two readings? What is the point of nullIndex? This feels wrong.
| for (const {term, reading, values} of map2.values()) { | ||
| for (let {term, reading, values} of map2.values()) { |
There was a problem hiding this comment.
It would be better to keep this const and define new variables when you need to modify stuff.
| let {currentAvg, count} = averages[term]?.[reading] ?? {currentAvg: 1, count: 0}; | ||
| if (valuesArray[0].frequency === null) { continue; } | ||
|
|
||
| currentAvg = (count / (currentAvg)) + (1 / (valuesArray[0].frequency)); |
| frequencies.push({ | ||
| term, | ||
| reading, | ||
| values: [...values.values()], | ||
| }); | ||
| const valuesArray = [...values.values()]; | ||
| if (reading === null) { reading = ''; } |
There was a problem hiding this comment.
This feels like the wrong way to handle this.
|
|
||
| :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; | ||
| } | ||
|
|
There was a problem hiding this comment.
This seems like a fragile way of setting this up. It would be better to have specific ids or a class.
|
It's probably also a good idea to split out the average calculation from the |
Kuuuube
added
kind/enhancement
this is a draft.
added a tag that displays the harmonic mean of the frequency ranks
updated a few things from #1303