fix: replace manual anomalies with a hampel filter#1997
fix: replace manual anomalies with a hampel filter#1997matthewp wants to merge 9 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
matthewp
changed the title
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR replaces blocklist-based anomaly correction with a Hampel-filter implementation: adds Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: beb9630b-e84a-4279-a4f8-362d18f451b4
📒 Files selected for processing (5)
app/components/Package/TrendsChart.vueapp/components/Package/WeeklyDownloadStats.vueapp/utils/download-anomalies.data.tsapp/utils/download-anomalies.tstest/unit/app/utils/download-anomalies.spec.ts
💤 Files with no reviewable changes (1)
- app/utils/download-anomalies.data.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/components/Package/TrendsChart.vue (1)
1831-1838: Consider usingv-modelfor simpler checkbox binding.The explicit
:checked+@changepattern is functionally correct, butv-modelprovides equivalent behaviour with less boilerplate.♻️ Optional simplification
<input - :checked="settings.chartFilter.anomaliesFixed" - `@change`=" - settings.chartFilter.anomaliesFixed = ($event.target as HTMLInputElement).checked - " + v-model="settings.chartFilter.anomaliesFixed" type="checkbox" class="accent-[var(--accent-color,var(--fg-subtle))]" />
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 91971e88-96ef-48e4-befd-3a0865a5eb4c
📒 Files selected for processing (17)
app/components/Package/TrendsChart.vuei18n/locales/bg-BG.jsoni18n/locales/cs-CZ.jsoni18n/locales/de-DE.jsoni18n/locales/en.jsoni18n/locales/es.jsoni18n/locales/fr-FR.jsoni18n/locales/hu-HU.jsoni18n/locales/id-ID.jsoni18n/locales/ja-JP.jsoni18n/locales/pl-PL.jsoni18n/locales/ru-RU.jsoni18n/locales/tr-TR.jsoni18n/locales/uk-UA.jsoni18n/locales/zh-CN.jsoni18n/locales/zh-TW.jsoni18n/schema.json
💤 Files with no reviewable changes (16)
- i18n/locales/en.json
- i18n/locales/bg-BG.json
- i18n/locales/es.json
- i18n/locales/de-DE.json
- i18n/locales/zh-TW.json
- i18n/locales/fr-FR.json
- i18n/locales/tr-TR.json
- i18n/locales/cs-CZ.json
- i18n/locales/hu-HU.json
- i18n/schema.json
- i18n/locales/zh-CN.json
- i18n/locales/uk-UA.json
- i18n/locales/ru-RU.json
- i18n/locales/ja-JP.json
- i18n/locales/pl-PL.json
- i18n/locales/id-ID.json
|
this looks very promising! tagging @jycouet who may have some thoughts on this 🙏 |
|
Great to have a second look into this. You probably checked my initial PR, #1636 I started with hampel implementation 👍 If I remember correctly, the sweet spot was 2.5 or 3 for vite. But this setting would hide "great start" of a lib. (eg: 0 0 0 0 0 0 20000) Another note, when the start or end of the chart is in an anomalie period, any algo can't fix it. A good example is: if you start the chart in the middle of the vite spike. Maybe we should add some tests around all this ? (to keep the intent) Another note 2: I would love to have more than just vite there today ! 😅 |
|
Thanks for this! A few thoughts after pulling the branch: Reproduction URL that matters: "Great start" test cases:Packages like PR suggestionCould we expose the Hampel filter as its own (separate from manual correction), with tweakable halfWindow / threshold sliders? That way we can compare manual vs Hampel side by side and find the right balance before committing to one approach? // Happy coding |
I don't follow, as its own what? |
Something like It's own controls. (It's more to be able to tests different senarios) |
|
Yes I can do that. |
Skip points without a full symmetric window to avoid flattening real
growth at series edges ("great start" problem). Use a relative check
when MAD is 0 instead of flagging any deviation, so sparse packages
like [0,0,0,1,0,0,0] keep their real activity.
Expose halfWindow and threshold as sliders on a second row in the data correction panel so users can tune the filter interactively.
matthewp
force-pushed
the
feat/hampel-anomaly-detection
branch
from
17e63ce to
88334f4
Compare
|
@jycouet I added sliders and addressed your concerns. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
i18n/locales/en.json (1)
469-477:⚠️ Potential issue | 🟡 MinorRename this section away from “Known anomalies”.
These strings now sit beside Hampel window/threshold controls, but they still describe a curated set of bot/CI spikes. The PR now applies automatic statistical filtering instead, so this wording misrepresents what the toggle does and how the values are changed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 848ad525-54b8-48c1-961d-cda990fce395
📒 Files selected for processing (23)
app/components/Package/TrendsChart.vueapp/components/Package/WeeklyDownloadStats.vueapp/composables/useSettings.tsapp/utils/download-anomalies.data.tsapp/utils/download-anomalies.tsi18n/locales/az-AZ.jsoni18n/locales/bg-BG.jsoni18n/locales/cs-CZ.jsoni18n/locales/de-DE.jsoni18n/locales/en.jsoni18n/locales/es.jsoni18n/locales/fr-FR.jsoni18n/locales/hu-HU.jsoni18n/locales/id-ID.jsoni18n/locales/ja-JP.jsoni18n/locales/pl-PL.jsoni18n/locales/ru-RU.jsoni18n/locales/tr-TR.jsoni18n/locales/uk-UA.jsoni18n/locales/zh-CN.jsoni18n/locales/zh-TW.jsoni18n/schema.jsontest/unit/app/utils/download-anomalies.spec.ts
💤 Files with no reviewable changes (15)
- i18n/locales/ja-JP.json
- i18n/locales/zh-TW.json
- i18n/locales/fr-FR.json
- i18n/locales/hu-HU.json
- i18n/locales/es.json
- i18n/locales/cs-CZ.json
- i18n/locales/pl-PL.json
- i18n/locales/de-DE.json
- i18n/locales/tr-TR.json
- i18n/locales/ru-RU.json
- i18n/locales/bg-BG.json
- i18n/locales/zh-CN.json
- i18n/locales/id-ID.json
- i18n/locales/uk-UA.json
- app/utils/download-anomalies.data.ts
✅ Files skipped from review due to trivial changes (1)
- i18n/locales/az-AZ.json
| hampelWindow: number | ||
| hampelThreshold: number |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "useSettings.ts" | head -5Repository: npmx-dev/npmx.dev
Length of output: 93
🌐 Web query:
VueUse useLocalStorage mergeDefaults shallow merge documentation
💡 Result:
In VueUse, useLocalStorage() uses the same options as useStorage(), and the mergeDefaults behavior is documented on the useStorage page. (vueuse.org)
mergeDefaults: truemerges the default value with the value read from storage, and for objects it does a shallow merge (top-level keys only). (vueuse.org)- For a deep merge, pass a custom function to
mergeDefaults(the docs explicitly show this pattern). (vueuse.org)
Example (custom merge = deep merge):
import { useLocalStorage } from '@vueuse/core'
// import deepMerge from '...'
const state = useLocalStorage('key', { a: { b: 1 } }, {
mergeDefaults: (storageValue, defaults) => deepMerge(defaults, storageValue),
})Citations:
- 1: https://vueuse.org/core/usestorage/?utm_source=openai
- 2: https://vueuse.org/core/usestorage/?utm_source=openai
- 3: https://vueuse.org/core/useStorage/?utm_source=openai
🏁 Script executed:
cat -n ./app/composables/useSettings.tsRepository: npmx-dev/npmx.dev
Length of output: 7350
🏁 Script executed:
# Search for migration or normalisation logic in settings/preferences
rg -i "migrat|normaliz|hydrat" --type ts --type tsx -A 3 app/composables/ 2>/dev/null | head -50Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
# Check usePreferencesProvider.ts for migration patterns
find . -type f -name "*usePreferences*" -o -name "*preferences*" | grep -E "\.(ts|tsx)$"Repository: npmx-dev/npmx.dev
Length of output: 236
🏁 Script executed:
head -150 ./app/composables/usePreferencesProvider.tsRepository: npmx-dev/npmx.dev
Length of output: 2609
🏁 Script executed:
# Search for any settings normalisation, migration, or post-hydration logic
rg "chartFilter|hampel" --type ts --type tsx -B 3 -A 3 app/ | head -100Repository: npmx-dev/npmx.dev
Length of output: 88
🏁 Script executed:
# Check if there's any middleware or plugin that normalises settings
rg "DEFAULT_SETTINGS|STORAGE_KEY|npmx-settings" --type ts --type tsx -B 2 -A 2 app/Repository: npmx-dev/npmx.dev
Length of output: 88
🏁 Script executed:
# Search for chartFilter and hampel references
rg "chartFilter|hampel" --type ts -B 3 -A 3 app/Repository: npmx-dev/npmx.dev
Length of output: 1232
🏁 Script executed:
# Search for settings normalisation anywhere
rg "DEFAULT_SETTINGS|STORAGE_KEY.*npmx-settings" --type ts -B 2 -A 2 app/Repository: npmx-dev/npmx.dev
Length of output: 885
Deep-merge nested chartFilter settings on hydration.
VueUse's useLocalStorage with mergeDefaults: true only performs a shallow merge at the top level. Returning users with an existing stored chartFilter object will not receive the new hampelWindow and hampelThreshold fields—the entire default chartFilter is replaced, leaving the new settings unset until explicitly written. VueUse documentation recommends a custom merge function for nested defaults.
Suggested fix
if (!settingsRef) {
settingsRef = useLocalStorage<AppSettings>(STORAGE_KEY, DEFAULT_SETTINGS, {
- mergeDefaults: true,
+ mergeDefaults: (storageValue, defaults) => ({
+ ...defaults,
+ ...storageValue,
+ connector: { ...defaults.connector, ...(storageValue.connector ?? {}) },
+ sidebar: { ...defaults.sidebar, ...(storageValue.sidebar ?? {}) },
+ chartFilter: {
+ ...defaults.chartFilter,
+ ...(storageValue.chartFilter ?? {}),
+ },
+ }),
})
}| // Only evaluate points that have a full symmetric window. | ||
| // Boundary points lack enough context on one side, making them | ||
| // prone to false positives (e.g., a "great start" at the end of a series). | ||
| for (let i = halfWindow; i < values.length - halfWindow; i++) { | ||
| const start = i - halfWindow | ||
| const end = i + halfWindow | ||
| const window = values.slice(start, end + 1) |
There was a problem hiding this comment.
Boundary spikes still leak back in through date-range trimming.
This loop never scores the first or last halfWindow buckets. Because Package/TrendsChart.vue applies Hampel after fetching the user-selected range, any anomaly that becomes a boundary point after cropping is guaranteed to survive, so narrowing the chart around a spike can reintroduce the very point the default view removed. Please run the filter on padded history and trim afterwards, and add a regression for a cropped-range edge spike.
|
@matthewp I pulled your branch, and see the 2 controls And with http://127.0.0.1:3000/package/vite?modal=chart&start=2025-03-13&end=2026-03-11 I get: 
I tried also, removing my local storage to start from "fresh", but I still see the big spike. I didn't look at the code at all, let me know how you want to proceed. |
|
@graphieros I think that the merge broke the test @matthewp I tried with your fe605db, If I understand your branch, you need to set Hampel window (4) + Hampel threshold (4) + Apply correction? Right?! And the apply correction is disconnected from the actual behaviorr. To be able to move forward, I would suggest to: My thinking is that without manual data, I don't see how to avoid this: |
Remove reference to unused variable
Sorry for that, removed the culprit line |
|
@jycouet Removing the biased manual data manipulation is the primary motivation of this pull request. So I'll keep this one until we can resolve whatever issues get us there. Is "Vite must look perfect at all times" really the primary requirement of this feature? Vs. correcting the unknown (hundreds? thousands?) of packages that likely have anomalies that are not corrected on npmx.dev currently? Currently Vite only looks bad if you enter a date range that no one is going to look at unless they are attempting to play with the controls. Tell me what the requirements are and I'll figure out a solution. |
|
@matthewp I understand that it's the motivation, and if we manage to have a better result than manual without manual, I'm all for it. I'm just trying to see how to test the feature! So my idea was to keep everything as is (where we can disable everything), and ADD this feature to be able to see behavior on multiple packages. It all started with vite, because it's obvious that there is an issue for a few weeks... But we now have a bit more: https://github.com/npmx-dev/npmx.dev/edit/main/app/utils/download-anomalies.data.ts
I think it's not only vite, and what I like with
Also when we look at the daily view
That's also a "big" decision, what default value to set, and the bigger the more we affect other parts.
Manual data is by package right now, but if one day we know something affecting ALL packages, we could also add this one info and all packages will have it 💪
I don't know what to answer here, you opend the issue & the PR ^^. I think that we are trying to do better things all together :)
|
|
I think acceptance criteria could be summed up as follows:
|
|
@jycouet I don't agree that the manual data manipulation is better, it misses an unknown number of packages and introduces bias into the entire site's data set. I didn't review or know about your PR when I submitted this one, to be clear, but I would assume that it did it better than the current state as well. I think looking at this by wanting specific packages to have nice looking charts is the wrong approach. You are devaluing having a consistent, unbiased method for detecting anomalies. Not having false positives is what is most important, some false negatives will happen, but it will be universally applied. I think the way to review a change like this is not focusing on specific desired results, but by looking 1) at the documentation on hampel detection and whether it's a good algorithm for this sort of problem. It's used within Matlab which I think is a good indicator that it is. And then 2) is the algorithm implemented correctly, is the code good, etc. I know that the "everyone chips in with what they know" feels warm and good, and for some things that's true, but when it comes to data it's not the case, everyone chipping in with their individual biases results in a distorted data set. Not maliciously, but biased nevertheless through selection bias, recall bias, survivorship bias, etc. As for the individual recent changes, I don't know what the correct default values should be, the original values seemed correct to me, I adjusted them based on the view you were looking at, but it wasn't completely clear to me what you were looking at as I do not get the same results locally. So happy to roll that back to previous values. But first I will reiterate that I am not sure what the requirements are here. I again ask, is it that Vite must never show this spike, regardless of what controls and date ranges are used? |
|
I would like some maintainer feedback please, cc @danielroe do you have views here, thanks. |
|
@graphieros Thank you, which of those criteria do you think are not passing currently? I think it's just (4), and for the date range in @jycouet's last screenshot, only for Vite. Is that correct? Or is there another? |
|
@matthewp it does appear like 4 is the current blocker, from what I see. Is the correction applied at the right stage of the data pipeline ? I think we need to define a set of packages to test against, like (probably non exhaustive):
|
|
Thank you @matthewp for sharing, I understand better your view. I ran some benchmark on: sorry for the flashbang 🤪 
|
|
what's needing to be done still? minus dealing with the merge conflicts :p cc: @graphieros @jycouet |
This PR looks like it is still pending. |
|
Hey @matthewp, thanks for pushing on this, it made me dig deeper Hampel again. I genuinely think a pure statistical approach is elegant, and in a perfect world I'd love to get there. But as the benchmarks showed, Hampel still struggles with real-world edge cases (great-start packages, boundary spikes on date range changes, low-volume data). These aren't minor => they affect how people read the charts. As the correction data is open, anyone can see what was corrected and why. Transparency seems like a better answer to the bias concern than removing human judgment entirely. I think that we can close this PR for now (or move as draft so that it's not in list of "to look"). |
Transparency in this case would explicitly labeling the correction data as a biased sample. Are you willing to do that? I'd still like to work on this PR, happy to move into a draft or close, whatever you prefer. Also open to a separate PR that removes the correction data entirely, so all packages are equally flawed, if you are open to that. |
|
The corrections are primarily based on known npm reporting events (outages, API bugs), not aesthetic preferences for specific packages. I wouldn't call that a biased sample -> it's closer to an errata log. I was thinking that it's a good thing for everyone, apparently not. I didn't mean to corrupt data, just to make charts more accurate when we know something went wrong on npm's side. When a known anomaly adds 100M+ downloads/week for over a month, it's not exactly a rounding error. Leaving that uncorrected feels like the bigger data integrity issue to me. Adding a Hampel option alongside the current corrections sounds great to me. Removing "Known anomalies" that I believe aren't destroying data, I'm less sure about, but I'm just an enthusiast who likes coding and learning here, nothing else. I'm sure the project will lean toward the best, no matter the direction, all good. |
|
I suggest we:
If Hampel (or another method) proves reliable across all scenarios, we can then later reduce reliance on manual corrections, or keep them only for extreme known events (?), but we should earn that transition with data, not assume it upfront. In the meantime, this PR can be converted to a draft, if this works for you. |
Known to a subset of people who contribute to this project, that's known as ascertainment bias. The act of cleaning the data is not neutral, it encodes the cleaner's subjective knowledge into the dataset. Also related is detection bias.
It's not, because the unknown anomalies are not corrected, so when users who visit 2 package pages, one corrected and one not, the uncorrected packages looks less trustworthy, and the user is more likely to favor the package with normal looking charts. So, unintentionally or not, putting bias into this product leads to the maintainers own packages (of which the maintainers are involved in many high profile projects) having a higher conversion rate than competing packages which have not been corrected. This is the alarming thing and why I originally signaled that the feature should just be removed. |
The manual correction is not reliable across all scenarios. One of the reasons I haven't been enthusiastic about coming back to this PR is because there are very high standards for a known anomaly correction mechanism used by MATLAB among other places (Hampel), and there is no standards for the known biased correction mechanism (manual). Can we apply some requirements for manual corrections in order to balance things out better? |
Sounds fair enough to me. |
|
Yeah something like that, also:
|
As decent as this would be for clinical data, it does not seem to be the case for npm downloads. Any manual correction is curated by npmx maintainers. Note that the few corrections applied correct obvious bursts, and the correction can be turned off to view the raw data, but we have been through this already. The source of truth remains available which is essential. This PR should be focused on improving the correction algorithm, and its coexistence with the manual correction is a requirement for now. |
|
@graphieros Happy to table that discussion. I'm a little confused though, you previously said
Are you now saying this isn't an acceptance criteria for this PR, but that it must also bring back the biased manual corrections? |
|
If the algorithm checks all the boxes of listed requirements, it would basically be redundant with manual corrections. I would applaud such a feat. Until then, manual correction will remain featured. |
5 participants
🔗 Linked issue
Previous issue: #1707
🧭 Context
The current implementation of anomaly removal is biased due to the manual nature.
This replaces the implementation with one that uses a hampel filter to automatically remove deviations.
📚 Description
The important part here is
applyHampelCorrection.It goes over each data point and creates a sliding window of data points, by default 3 on each side.
Of those data points it finds the median, since a spike can't pull it off center.
Then it measures how spread out the neighbors are by calculating the MAD (see here).
Then it gives the data point a score and if it's above a threshold then it gets replaced with the median of that window and marked as being an anomaly.
Here are two articles about how hampel filters work:
Here's a screenshot of Vite which still shows its spike removed after this change.