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

Improve support for SCAT dataset #437

Merged
merged 6 commits into from
May 24, 2024
Merged

Improve support for SCAT dataset #437

merged 6 commits into from
May 24, 2024

Conversation

niclaswue
Copy link
Contributor

As discussed here I added the engine flag to the eval calls. While I was at it, I also implemented some functions for loading the waypoints and weather data. Existing code should not break as I added the additional flags include_weather and include_waypoints and set them to False by default.
Both weather and waypoints are just dataframes right now because there is some information missing from the waypoints which would be needed to make them Navaid objects. The weather data is decoded grib format which is different from the implemented metar data format.

Let me know what you think!
Thanks

@xoolive
Copy link
Owner

xoolive commented May 17, 2024

Thank you so much,

With reflexion, I see another alternative here: how about changing the .eval(..., engine="python") with an .assign(... = lambda df: ..., ) ? What do you find is more clear? I am more inclined to prefer the lambda option: eval is great, but if we need the engine kwarg, it is not true anymore.

Also about the test, do you mind writing something slightly more explicit for weather and waypoints, like the presence of some column in the dataframe? or an equality for the max of some value in a given column? This helps to ensure we get the proper columns we expect with these methods...

@niclaswue
Copy link
Contributor Author

Yes, good call. I think assign was meant for this. I believe it would make error messages and thus debugging much easier.

Sure, I can write some better tests when I’m back on office on Tuesday.

@niclaswue
Copy link
Contributor Author

I added some real test cases now. However, I wondered if just returning the waypoints as a large dataframe is really the best option, here. We do not have information other than latitude, longitude, name in the dataset but it would be nice to have a list of Navaid objects which in turn could be used for parsing the flight plans, if that is possible?

Additionally, SCAT provides data to describe the airspace which could be parsed as well, I think. Let me know if you would want to keep this all in one PR or split it into multiple PRs.

@niclaswue
Copy link
Contributor Author

I dove a bit deeper into the navaids and rewrote it so that the scat dataset holds the points as a Navaids object. This way the user could do something like this:

from traffic.data import navaids
from traffic.data.datasets.scat import SCAT

print(navaids.get("VISI"))  # None
print(navaids.get("ZR1"))  # None

scat = SCAT("scat20161112_20161118.zip", nflights=50, include_waypoints=True)
navaids.alternatives["SCAT"] = scat.waypoints

print(navaids.get("VISI"))  # Navaid('VISI', type='FIX', latitude=58.1, longitude=14.4)
print(navaids.get("ZR1"))  # Navaid('ZR1', type='FIX', latitude=59.0, longitude=16.0)

in my opinion, this would be quite clean and more useful as it also helps with parsing the flight plans.

@xoolive xoolive merged commit 7fefd31 into xoolive:master May 24, 2024
4 checks passed
@xoolive
Copy link
Owner

xoolive commented May 24, 2024

Thanks!

@xoolive
Copy link
Owner

xoolive commented May 24, 2024

I just fixed the tests 😱
Also maybe it would make sense to append .to_xarray() for the weather dataframe, but on the other hand it is an optional dependency so... 🤷 maybe not...

@niclaswue
Copy link
Contributor Author

Hey there, thanks for merging! Sorry, the tests were still for the dataframe implementation, forgot to mention that in my comment 🙄. I am not familiar with xarray and its advantages, if it is only a .to_xarray() call, I believe it would also be ok to leave it to the user to convert it if necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants