-
Notifications
You must be signed in to change notification settings - Fork 162
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
this fixes (papers over) issue #547. #608
Conversation
…s, which remove transmissions from the index, which later results in 'type undefined' errors.
Hey @rneher -- could you run these JSONs through I'm happy enough with a temporary solution to stop things crashing, but this is a deeper seated issue in the design of the map component which assumes that every value has a corresponding lat-long. I guess we should (a) not show links to and from "unknown" demes (unknown in the sense that their lat/long is unknown) and (b) display an error to the user. |
fails, but for the wrong reason:
|
Thanks... i'll fix |
note that these were the ancient jsons that caused this issue back in May. Don't even know where they came from, you posted them on slack. My main motivation was to shorten the long issue list of auspice. |
I'm in favor of merging this. Having lots of redundancy in auspice seems like the way to go. Even if these JSONs won't validate, still better to have auspice not crash. |
I'm confused, on current I'm very much in favor of (re-)displaying the error message, but not displaying transmissions/demes seems nicer than showing ones that have a fake location. |
I see. I'm agnostic in this case. |
I tend to agree that not displaying them is better than drawing to some place in the Atlantic. But it is difficult for the user to know that smth is amiss if only a few lines are missing. My main reason to go back to this was to figure out where exactly this goes wrong for what reason. We had previously just caught it right before drawing. The root cause is info in the meta_json and that previously resulted in a corrupted the transmission data structure. |
My take is that the transmission should exist in the data structure since it is in the json, but it should have coordinates |
Augur PR nextstrain/augur#192 modifies
|
Here's a possible fix (feel free to trash a1c66ac if it's not desired): Demes/transmissions involving undefined lat/longs are not rendered & this error notification is shown on page load: |
My main reason to get back to this was to understand why this error happened and to reduce the number of |
Great! |
The issue was missing coordinates which result in missing transmissions from the index, which later results in 'type undefined' errors. This PR just sets lat/long to 0.0 instead. This will result in silly locations, but at least it doesn't break. This is a more general issue as to what should happen with incomplete geo-info.