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

Fixes #4816 - Fix incorrect VOR/NDB Coordinates #4817

Merged
merged 12 commits into from
Jul 26, 2023
Merged

Fixes #4816 - Fix incorrect VOR/NDB Coordinates #4817

merged 12 commits into from
Jul 26, 2023

Conversation

AliceFord
Copy link
Collaborator

@AliceFord AliceFord commented Jul 18, 2023 •
edited
Loading

Fixes #4816

Summary of changes

  • Fix incorrect VOR/NDB coordinates (NDB only stations not included in this PR)

Screenshots (if necessary)

@AliceFord AliceFord self-assigned this Jul 18, 2023
@AliceFord AliceFord added the bug Bugs or errors label Jul 18, 2023
@AliceFord AliceFord marked this pull request as ready for review July 18, 2023 06:29
chssn
chssn approved these changes Jul 18, 2023
View reviewed changes
Copy link
Collaborator

@chssn chssn left a comment

Choose a reason for hiding this comment

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

all changes match airac 07/2023

@PLM1995
Copy link
Collaborator

PLM1995 commented Jul 18, 2023

all changes match airac 07/2023

@chssn Thanks very much for reviewing! Just for any future reviews you do, it's best to review against the upcoming AIRAC data (2308 in this case) rather than the current one, in case there are any upcoming changes which will take effect before the next sector file gets published :) Looking at the AMDT I don't think there are here.

@TechieHelper I assume this was done programatically? Would it be worth adding the software you used to our repo do you think? For want of a better suggestion maybe under _data/Tools/ENR4.1parser or something, potentially also with a little instructions.md if necessary, so it could be updated/used in future by anyone? Same might apply to any other stuff you're automating the update of.

And did you use the 2308 or 2307 AIRAC AIP?

Great work as ever!

Note to self / other reviewers: now ordered alphabetically by station name (not ID), was a mixture before. This matches the AIP.

@AliceFord
Copy link
Collaborator Author

AliceFord commented Jul 18, 2023 •
edited
Loading

Yep it was programmatically generated, I'll get all the software up soon on a commit, just going to clean it up a bit first. It was the 2307 AIRAC used for this but it's extremely easy to change to 2308 for future use.

chssn
chssn requested changes Jul 18, 2023
View reviewed changes
Copy link
Collaborator

@chssn chssn left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I have a (somewhat outdated) eAIP scraper here - https://github.com/chssn/uk-dataset/blob/master/ConversionTools/generate.py which is where the AIRAC class referenced below has come from.

_data/Tools/ENR4.1parser.py Outdated Show resolved Hide resolved
_data/Tools/ENR4.1parser.py Outdated Show resolved Hide resolved
_data/Tools/ENR4.1parser.py Outdated Show resolved Hide resolved
_data/Tools/ENR4.1parser.py Outdated Show resolved Hide resolved
@AliceFord
Copy link
Collaborator Author

Hiya @chssn, thanks for that, I'm planning to make a full AIP scraper API in the future, so this will come in handy! Thanks :)

@chssn
Copy link
Collaborator

chssn commented Jul 18, 2023

Hiya @chssn, thanks for that, I'm planning to make a full AIP scraper API in the future, so this will come in handy! Thanks :)

No problem, I have got a semi-updated version as a package somewhere... I'll give you a shout when I track it down!

@AliceFord AliceFord mentioned this pull request Jul 18, 2023
Merged
@chssn
Copy link
Collaborator

chssn commented Jul 18, 2023

Hiya @chssn, thanks for that, I'm planning to make a full AIP scraper API in the future, so this will come in handy! Thanks :)

No problem, I have got a semi-updated version as a package somewhere... I'll give you a shout when I track it down!

Here you go - https://github.com/chssn/eaip-parser - completely unvalidated at the moment so no guarantees it does what it says it does but it's a starting point!

@AliceFord
Copy link
Collaborator Author

Ah thank you, that's some very nice code you've got there! It'll definitely come in very handy :)

PLM1995
PLM1995 reviewed Jul 19, 2023
View reviewed changes
Copy link
Collaborator

@PLM1995 PLM1995 left a comment

Choose a reason for hiding this comment

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

Good stuff as far as I can tell, all the output data seems correct, just a couple of queries/suggestions on the python.

_data/Tools/Airac.py Outdated Show resolved Hide resolved
_data/Tools/ENR4.1parser.py Outdated Show resolved Hide resolved
_data/Tools/ENR4.1parser.py Outdated Show resolved Hide resolved
AliceFord and others added 4 commits July 19, 2023 09:41
@AliceFord @PLM1995
Update ENR4.1 parser
4d7045a 
Co-authored-by: Peter Mooney <[email protected]>
@AliceFord @PLM1995
Update AIRAC class
fd8044f 
Co-authored-by: Peter Mooney <[email protected]>
@AliceFord
Update DME naming
3fc5c64
@AliceFord
Completely update ENR4.1 parser
3ea76f3 
- Update parser
- Create backbone for further parsers
- Add some testing (more to follow)
@AliceFord AliceFord marked this pull request as draft July 19, 2023 09:21
@AliceFord
Copy link
Collaborator Author

Back to draft for finishing up tooling

@AliceFord AliceFord marked this pull request as ready for review July 19, 2023 09:35
PLM1995
PLM1995 reviewed Jul 22, 2023
View reviewed changes
Copy link
Collaborator

@PLM1995 PLM1995 left a comment

Choose a reason for hiding this comment

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

@chssn Happy with how this looks now? I think it's changed a fair bit since your origional approval. I haven't studied the tools stuff.

_data/Tools/tests/test_api.py Show resolved Hide resolved
chssn
chssn requested changes Jul 22, 2023
View reviewed changes
_data/Tools/src/util/airac.py Show resolved Hide resolved
_data/Tools/src/util/airac.py Show resolved Hide resolved
_data/Tools/src/util/airac.py Show resolved Hide resolved
_data/Tools/src/util/airac.py Outdated Show resolved Hide resolved
_data/Tools/src/util/airac.py Show resolved Hide resolved
_data/Tools/src/util/util.py Outdated Show resolved Hide resolved
_data/Tools/tests/test_util.py Outdated Show resolved Hide resolved
chssn
chssn approved these changes Jul 23, 2023
View reviewed changes
Copy link
Collaborator

@chssn chssn left a comment

Choose a reason for hiding this comment

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

looks all good to me - agreed with leaving self.cycles as they were. doesn't make a huge difference computationally and if it makes the testing more straight forward then go for it!

PLM1995
PLM1995 approved these changes Jul 26, 2023
View reviewed changes
Copy link
Collaborator

@PLM1995 PLM1995 left a comment

Choose a reason for hiding this comment

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

Haven't studied the code, but two other approvals and keen to get this merged so other tooling can be incorporated.

Thanks @AliceFord !

@PLM1995 PLM1995 merged commit f050bf3 into VATSIM-UK:main Jul 26, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chssn chssn chssn approved these changes

@Robbo599 Robbo599 Robbo599 approved these changes

@PLM1995 PLM1995 PLM1995 approved these changes

Assignees

@AliceFord AliceFord

Labels
bug Bugs or errors
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix incorrect VOR/NDB Coordinates
4 participants
@AliceFord @PLM1995 @chssn @Robbo599