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.

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

"Zero wind" (00000KT) scenario not completely handled #61

Closed
SkySails opened this issue Dec 28, 2023 · 4 comments · Fixed by #63
Closed

"Zero wind" (00000KT) scenario not completely handled #61

SkySails opened this issue Dec 28, 2023 · 4 comments · Fixed by #63
Assignees
Labels
bug Something isn't working

Comments

@SkySails
Copy link

Hello! First of all, thank you for your awesome work on this family of parsers 🙌

I am debugging an issue in the Typescript port of this project, and it seems like the same thing occurs over here and since that other project is a port, I think it's best to work with this upstream first!
The following METAR report incorrectly parses to the wrong unit:

KATL 270200Z 00000MPS

The result, as per https://www.metar-taf-decoder.com, is Wind: Speed : 0 KT Direction : North | 0° - KT instead of MPS.
This is a really small issue, but it has other consequences in the Typescript port which you can read about in the linked issues at the bottom!

Related issues

More details

The culprit seems to be this regex, as it results in the following incorrect matching:

regex = r'^(VRB|00|[0-3]\d{2})(\d{2})G?(\d{2,3})?(KT|MPS|KM\/H)?'

It seems to be there to satisfy this test case, which includes an invalid METAR string according to the specifications I can find. As usual, most are not too explicit on the matter but this is one example:

Wind speeds (ff) are two digits (or three digits if required), in knots.
https://meteocentre.com/doc/metar.html

So I guess this is a case of choosing which case to support. If we change the regex by adding an additional required zero:

(VRB|000|[0-3]\d{2})(\d{2})G?(\d{2,3})?(KT|MPS|KM\/H)?

The unit is now correctly parsed and the speed is correctly registered (not that it matters, it's all zeros anyway)

image

The downside is that the test case will start to fail. Is there a reason behind supporting 0000KT (four digits) specifically? Is it a common occurrence in the real world?

@mivek mivek self-assigned this Dec 28, 2023
@mivek
Copy link
Owner

mivek commented Jan 13, 2024

Hello @SkySails

Sorry for the delay.
The support for the 0000KT token actually comes from a PR from the Typescript project:

d) Calm Wind. Calm wind shall be coded as "00000KT"
The first three zeros represent the wind direction and the last two zeros represent the speed.

(VRB|000|[0-3]\d{2})(\d{2})G?(\d{2,3})?(KT|MPS|KM\/H)?

is the right regex.

Thank you for your analysis of the bug, I will push a fix.

@mivek mivek added the bug Something isn't working label Jan 13, 2024
@SkySails
Copy link
Author

No worries at all!

Ah, I should have looked through the issue history too, that's my bad. Thank you for referencing it!
You're welcome, I'm happy to help!

mivek added a commit that referenced this issue Jan 14, 2024
This PR fixes #61 and the regex of the calm wind which is represented by `00000KT` and not `0000KT`.
Visibility should not be parsed aas wind.
@mivek mivek mentioned this issue Jan 14, 2024
@mivek mivek linked a pull request Jan 14, 2024 that will close this issue
@mivek mivek closed this as completed in #63 Jan 14, 2024
@mivek
Copy link
Owner

mivek commented Jan 14, 2024

Hello @SkySails

I merged the fix and released a new version 1.8.2. I will report the fix in the Java version in the next few days.

Thanks again for reporting the issue

@SkySails
Copy link
Author

That's great news! Thank you for fixing it!

mivek added a commit to mivek/MetarParser that referenced this issue Jan 15, 2024
This is a fix to mivek/python-metar-taf-parser#61. The calm wind was parsed as a visibility token.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants