-
-
Notifications
You must be signed in to change notification settings - Fork 23
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 anomalist to owidbot #3431
Conversation
Quick links (staging server):
Login: chart-diff: ✅No charts for review.Edited: 2024-10-18 12:30:07 UTC |
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.
Looks good! Your way to detect new datasets is very simple and it makes sense. Two comments (probably better to discuss on slack):
- If this is meant to happen under the hood, then it would make sense to infer the variable mapping if indicator upgrader has not yet been executed, which is what I implemented on ✨ anomalist: Improve automatic detection of new datasets #3429
- It's not clear to me what should happen now if the user opens Anomalist. We need to adapt the app so that anomalies are automatically detected from database, or is that already working somehow? I suppose that loading anomalies for the new datasets from the start is the best option. And, if the user wants to check anomalies on other datasets, that should still be possible (but it will take a while to recalculate anomalies).
It's not working yet. You're right that it should select new datasets by default and load anomalies without needing to click on the button. I'll add it either in this PR or the next one. |
7432a6f
to
df4f867
Compare
df4f867
to
95e600d
Compare
dataset.sourceChecksum
toanomalies
table. This means we can skip computing anomalies unless a dataset changes (this is especially useful for owidbot and running anomalist on staging servers)_load_datasets_new_ids
to get new datasets (if it works, we can use it in anomalist app)