-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Adding altitude component to the Geospatial Coordinate Encoder #1606
Adding altitude component to the Geospatial Coordinate Encoder #1606
Conversation
Updated the GCE input tuple. Moved speed to position 0 and added optional altitude parameter to the end. Updated docstrings.
If altitude is specified, GCE will use a geocentric 3D coordinate system to encode longitude, latitude and altitude. Otherwise, it will use the same encoding system is has been using so far to encode longitude and latitude.
Added basic unit tests for validating coordinates generated by the 3D geocentric coordinate system when altitude is given as parameter.
@chetan51 Please review. More on the mailing list about this from @hernan-erasmo. Thank you, @hernan-erasmo! /cc @c4oe |
These tests try to make a little more clear how the GCE encodes latitude, longitude and altitude.
coords = PROJ(longitude, latitude) | ||
|
||
if altitude is not None: | ||
coords = transform(PROJ,geocentric,coords[0],coords[1],altitude) |
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.
Nitpick: spaces after commas
@hernan-erasmo This is wonderful work, very professional. Thank you! Before merging, I would like to see this tested end-to-end in our existing geospatial demo to make sure it gives the same results, and ideally tested end-to-end with a dataset that includes altitude to make sure that the new logic really does work. |
@hernan-erasmo I'm happy to provide additional data (aviation position reports) for testing, if that would help you. |
@chetan51 No problem, thank you for the feedback. I think I'm going to have to make some changes to the geospatial demo application, since it still uses the old interface (it doesn't work right now with the changes to the GCE, ie: this is not the correct order of arguments anymore. I'm going to fork the repo and then make the necessary changes and test it. @c4oe The data you provided has been very helpful. I've already extracted the most useful tracks from it (I discarded the ones with no latitude, longitude or altitude data) and used it to make a couple of test tracks that I think will be enough to test this new feature. However, the file you uploaded contains several different short paths. It would be great to have more data on a single specific route, for example, several aircraft flying through the same path at different altitudes (I believe that's the kind of data you want to process with nupic). By the way, I have a question regarding the 'mode-s hex id' field. I assume that it's a unique identifier of an aircraft (or flight). So if I have an entry that indicates that 'mode-s hex id' 00AA3F goes from point A to point B, and 3D0ABB goes from point B to point C, then 00AA3F and 3D0ABB are different flights, right? Thank you for your help. ps. Added an image of the tracks made with data extracted from the .csv @c4oe provided. Remember that points with no latitude, longitude or altitude data available have been removed. I guess that's why some paths end abruptly and then continue several miles away. |
@hernan-erasmo Please send a pull request with changes to update the geospatial demo as well; we don't want to break any clients using this encoder when we merge. |
@chetan51 What's the status of this PR at this point? Thanks. |
Looks good to merge now. Thanks! 👍 |
@hernan-erasmo Please update the CHANGELOG by adding a line describing the new feature and I'll merge. Great work! |
@rhyolight Sorry, but I just saw the message you sent to the mailing list where you say that it is no longer necessary to update the CHANGELOG manually. Do I still need to update it? If it's needed then I have no problem in doing it, but I wanted to check with you first since you wrote that mail after your comment. Thank you. |
…titude Adding altitude component to the Geospatial Coordinate Encoder
…odes-altitude Adding altitude component to the Geospatial Coordinate Encoder
Fixes #1461
Since the Coordinate Encoder doesn't have an upper limit on the number of dimensions of the coordinates, I think that using a 3D geocentric coordinate system to encode longitude, latitude and altitude should work.
This is the solution I propose: