-
Notifications
You must be signed in to change notification settings - Fork 28
Support parsing HyperlocalExperiment #276
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
base: main
Are you sure you want to change the base?
Conversation
|
Thanks for your contribution. I've got some questions about this that I hope you can help me with before any formal review
|
Spawns are captured conventionally but will be impacted by whether they are in the region or outside. In the past (during elite/t4 raid bonus), extra spawnpoints are activated inside the region and the spawn is drawn from a different pool. All the changes are reverted after the timer expires. It's like a mini-event but only for spawns (/players?) inside the circle. ReactMap could display these circles and Poracle could probably alert the player that a tracked spawn is inside the bonus region and is subject to change after the region expires/reactivates.
This could be in-memory only like Pokemon since they are all short lived. No idea about distance.
ID (3578) + challenge key (
No. This should be treated like Pokemon table. |
|
On the first point, I can see it being useful if we can determine the change time (either it's going to change, or it's going to change back). The "going to change" might be difficult for us to determine although it's likely s2 related rather than exactly circular. I wonder if there is anything on the spawn that indicates it is associated with the experiment that would allow correlation |
|
I don't think there is but I haven't checked, and I believe it could be exactly circular given the small radius (I think the biggest radius was 300m?). |
|
Can we merge this now that we have initial ReactMap support? :) |
|
Now that we have caching and cleanup, can we merge? 👀 |
|
I will review tomorrow and give comments. It will need community testing and verification also before merge |
|
Unfortunately, testing will be difficult though. There are no future announced events (yet) that use this feature, and the last one was three months ago. |
|
It can wait until then. If there are no more it isn’t relevant anyway :-) |
| Lon float64 `db:"lon" json:"lon"` | ||
| RadiusM float64 `db:"radius_m" json:"radius_m"` | ||
| ChallengeBonusKey string `db:"challenge_bonus_key" json:"challenge_bonus_key"` | ||
| UpdatedMs int64 `db:"updated_ms" json:"updated_ms"` |
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.
Interested in others thoughts about moving updated to a ms field (where others are not). Consistency might be more important than other concerns (I'd be willing to consider a PR moving to a virtual updated and every other table being updated to ms as the per second resolution annoys me too)
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.
I think it is fine to just do this instead to keep compatibility. I imagine this would break compatibility for a lot of other things if we introduce this to older tables though?
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.
You would use a virtual column (as a calculated field) to maintain the old name. Anyway, for the time being we should change this to updated
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.
Why? This is a new table and RM PR is already implemented for this. Do you mean we should add a virtual updated column for this instead?
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.
Also since the column explicitly gives the unit (which is actually a convenient consequence of following the same naming convention as proto), I think it's fine to just keep this as is.
| func (h *Hyperlocal) getKey() HyperlocalKey { | ||
| return HyperlocalKey{ | ||
| ExperimentId: h.ExperimentId, | ||
| Lat: h.Lat, |
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.
Urgh. I understand there may be no unique identifier (unusual for niantic though this is!) but I wonder if lat/lon as a float works reliably given the round-trip to database (I doubt it will be guaranteed byte-exact). We may be forced for the key to move to an integer representing a fixed precision - but I haven't seen the resolution of the data that comes from niantic for this - how many decimal places is it?
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.
I think it should be exact. From my data gathered three months ago, these fields are always truncated to 1e-6 (ignoring floating point rounding), e.g. 20.681438,-156.442944.
Full row: 3578,1743375172270,1743376072270,20.681438,-156.442944,100.00000000000001,spawn_wild_bug_types,1743376070600
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.
I guess if testing turns out to be problematic, we can store (int)(1e6*lat) instead. (It didn't seem problematic in the data I collected.)
| old.EndMs != new.EndMs || | ||
| old.RadiusM != new.RadiusM || | ||
| old.ChallengeBonusKey != new.ChallengeBonusKey || | ||
| old.UpdatedMs < new.UpdatedMs |
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 is not considering lat/lon - there is a method for nearby precision comparison of floats
updated wouldn't normally be considered here I don't think
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.
They aren't considered because they are primary keys. :)
| Lon: raw.Data.GetLngDegrees(), | ||
| } | ||
|
|
||
| hyperlocalMutex, _ := hyperlocalStripedMutex.GetLock(uint64(key.ExperimentId) ^ math.Float64bits(key.Lat) ^ math.Float64bits(key.Lon)) |
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.
I wonder if the hash would be better as a method on the key - ... I think the stripedmutex just calls the standard hashing interface from memory
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.
stripedmutex takes a string key so this is more performant.
| infoData["pokemon_id"] = int(info.GetPokemonId()) | ||
| } | ||
| if info.ShinyProbability > 0.0 { | ||
| if info.ShinyProbability != 0.0 { |
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.
why? equality is generally best avoided with floats (though 0 generally works as is a special float)
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.
These are unlikely to be a result of a floating point math calculation so I think precise comparison is fine. Also the old code is comparing with 0 as well instead of something like > 1e-12.
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.
And yes 0.0 is a special float in proto, indicating default value.
| PRIMARY KEY(`experiment_id`,`lat`,`lon`), | ||
| KEY `ix_end_ms` (`end_ms`), | ||
| KEY `ix_updated_ms` (`updated_ms`) | ||
| ) ENGINE = InnoDB |
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.
We don't normally force engine (some people use alternates), charset or collate (forcing and not using database default gives problems on a join)
Suggest that you probably need an index on lat,lon,end_ms for the standard map type query
and don't need ix_updated_ms as this won't be used anywhere
the end_ms is likely used to expire
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.
I think I copied this ENGINE part from a similar sql migration script.
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.
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.
I will PR the removal of these, this is a mistake

Fixes #272.