Skip to content
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.

Sign up for GitHub

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

Jump to bottom

geolocation: update to use GeoJSON #135

Merged
merged 1 commit into from
Jul 29, 2021
Merged

geolocation: update to use GeoJSON #135

merged 1 commit into from
Jul 29, 2021

Conversation

jacobagilbert
Copy link
Member

@jacobagilbert jacobagilbert commented Jun 17, 2021 •
edited
Loading

This closes #73 and closes #84 by identifying how altitude data should be included in recordings, and codifies the use of GeoJSON for the new global object core:geolocation.

This also marks the core:latitude and core:longitude fields in annotations for deprecation in a future (breaking) release.

@jacobagilbert jacobagilbert requested review from bhilburn and 777arc June 17, 2021 15:03
@jacobagilbert jacobagilbert added clarification a wording change in the spec that clarifies meaning enhancement labels Jun 17, 2021
Teque5
Teque5 requested changes Jun 19, 2021
View reviewed changes
Copy link
Collaborator

@Teque5 Teque5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, but I think we should remove the latitude and longitude keys instead of marking them as depricated.

Also remember to update the schema.json. I think also in validate.py:validate_key() you will have to add the geometry type.

@jacobagilbert
Copy link
Member Author

@Teque5 In general I agree but removing latitude and longitude keys would be a breaking change, and if I am recalling correctly (@bhilburn help me out here) we have decided that v1.0 should not break the v0.x spec (and I am aware of current software products using them). I need to update this per discussion in #73 and I'll update the other things you pointed out, thanks.

@jacobagilbert jacobagilbert mentioned this pull request Jun 29, 2021
Closed
bhilburn
bhilburn requested changes Jul 1, 2021
View reviewed changes
Copy link
Contributor

@bhilburn bhilburn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great PR, @jacobagilbert. I left a lot of comments / questions, but that's because this is a super complicated topic and I want to make sure we cover the edge cases everywhere we can. It is basically closing 4 years of discussion, so, not unexpected 😄

@Teque5 - I agree, and would prefer to just remove those deprecated fields entirely. The limiting thing, here, is that we do want to maintain backwards compatibility between v1.0.0 and v0.0.1, because we know v.0.0.1 is so widely used, and we don't want to break all those systems without giving them a stably versioned release.

sigmf-spec.md
@@ -293,8 +294,30 @@ the `global` object:
|`recorder`|false|string|The name of the software used to make this SigMF recording.|
|`license`|false|string|A URL for the license document under which the recording is offered; when possible, use the canonical document provided by the license author, or, failing that, a well-known one.|
|`hw`|false |string|A text description of the hardware used to make the recording.|
|`geolocation`|false|GeoJSON or string|Location of the recording system.|
|`hagl`|false|double|Antenna height above ground level (in meters).|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is "HAGL" described meaningfully somewhere that we could refer to? Honest question. @dharasty?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"...(in meters) as measured by Ottoman calipers from the phase center of the aperture to the mean surface level of the normal square decimeter in the direction of the center of the WGS84 ellipse"

:)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we want that extended description of HAGL added to line 298? I don't feel strongly about it either way.

Copy link

@dharasty dharasty Jul 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is "HAGL" described meaningfully somewhere that we could refer to? Honest question. @dharasty?

Alas, I searched high and low for something like an authoritative glossary (such as in an FCC document) containing "HAGL" defined or at least spelled out. To my amazement -- couldn't find one.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that HAGL is a generally informal term, I find what is in the PR to be sufficient.

@jacobagilbert
Copy link
Member Author

jacobagilbert commented Jul 1, 2021 •
edited
Loading

@bhilburn I think I have resolved what you were asking about (ok..not HAGL i'll let someone else chime in there...). I reference the RFC texts in only one place and used the "point object" language anywhere it is described to make sure that is clear. If something here isn't resolved let me know.

I also put this through a spell checker given my plethora of mistakes and fixed a couple others.

@jacobagilbert jacobagilbert force-pushed the update_geo branch 2 times, most recently from 5329c60 to 6c56c1a Compare July 1, 2021 12:21
@jacobagilbert jacobagilbert mentioned this pull request Jul 1, 2021
Merged
@gmabey
Copy link
Contributor

gmabey commented Jul 19, 2021

@777arc are you able to review this?

777arc
777arc approved these changes Jul 19, 2021
View reviewed changes
Copy link
Member

@777arc 777arc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I added a note about HAGL but don't let that keep it from getting merged in

sigmf-spec.md
@@ -293,8 +294,30 @@ the `global` object:
|`recorder`|false|string|The name of the software used to make this SigMF recording.|
|`license`|false|string|A URL for the license document under which the recording is offered; when possible, use the canonical document provided by the license author, or, failing that, a well-known one.|
|`hw`|false |string|A text description of the hardware used to make the recording.|
|`geolocation`|false|GeoJSON or string|Location of the recording system.|
|`hagl`|false|double|Antenna height above ground level (in meters).|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we want that extended description of HAGL added to line 298? I don't feel strongly about it either way.

@bhilburn
Copy link
Contributor

@jacobagilbert - New changes look great, but merge is blocked by conflict. Can you push an update resolving the conflict? Also, since you'll be in there, can you make your reference to RFC 7946 actually link to that IETF doc?

Once merge conflict is resolved, we are good to go here 😁

@bhilburn bhilburn mentioned this pull request Jul 28, 2021
Open
@jacobagilbert jacobagilbert force-pushed the update_geo branch 2 times, most recently from babe0b4 to 532e820 Compare July 28, 2021 20:42
@jacobagilbert
Copy link
Member Author

Should be G2G @bhilburn if you want to take one more quick look at the schema since I had to manually resolve that conflict that would be great.

@jacobagilbert jacobagilbert force-pushed the update_geo branch 3 times, most recently from 382b97f to afd32b4 Compare July 28, 2021 22:38
geolocation: update to use GeoJSON
480176d 
this makes system location a 'global' object and switches
the format to GeoJSON instead of discrete fields

Signed-off-by: Jacob Gilbert <[email protected]>
@bhilburn bhilburn merged commit 206f2a1 into sigmf:master Jul 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dharasty dharasty dharasty left review comments

@bhilburn bhilburn bhilburn requested changes

@Teque5 Teque5 Teque5 requested changes

@777arc 777arc 777arc approved these changes

Assignees
No one assigned
Labels
clarification a wording change in the spec that clarifies meaning enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

'altitude' field, and being able to put coordinates in global lat and long in Annotations
6 participants
@jacobagilbert @gmabey @bhilburn @Teque5 @dharasty @777arc