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

Support threshold coordinate projection via compute_xy() interface #326

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

sfo
Copy link
Contributor

@sfo sfo commented Feb 27, 2023

This PR intends to add support for projecting threshold coordinates using the common compute_xy() interface.

However, currently the RunwayAirport() constructor just swallows the enriched DataFrame passed to it at the end of compute_xy().
Consequently, the following code returns None:

>>> from traffic.data import airports
>>> airport = airports["CDG"]
>>> airport.runways.compute_xy()
None

The DataFrame containing the projection can be accessed via

>>> airport.runways.compute_xy()._data
    latitude  longitude     bearing name            x            y
0  48.995701    2.55274   85.293092  08L  -722.812433 -1562.345186
1  48.998798    2.61018  265.336441  26R  3480.306639 -1216.826055
2  48.992901    2.56566   85.263542  08R   222.653855 -1873.773350
3  48.994900    2.60243  265.291290  26L  2913.454043 -1650.716705
4  49.024700    2.52489   85.264343  09L -2759.230258  1663.306856
5  49.026699    2.56169  265.292128  27R   -67.826773  1884.919854
6  49.020599    2.51306   85.267941  09R -3624.711297  1207.753756
7  49.023701    2.57029  265.311147  27L   561.147681  1551.500392

I am currently not sure how to propagate the results of the projection back up the runway/threshold data stream.

@xoolive
Copy link
Owner

xoolive commented Feb 28, 2023

Thank you for the update.

Indeed, that structure is not great now 😢
It would need a serious refactoring that has not been done yet (and it's too late for tonight 😇 to start that!)

It's not very natural to model the whole list of runways with a dataframe because the thresholds come by two (and you need the two thresholds to plot the whole runway for instance)

The .data attribute is not an attribute but a property, which computes a view on which you use .compute_xy() to get the x,y parameters, build a new dataframe but the proper way to instantiate a RunwayAirport class should be from a list of pairs of thresholds.

This is obviously not great, but I never took time to think about how to improve it.

  • The full DataFrame approach (using the .data view as the base structure) breaks the link between the thresholds.
  • the GeoDataFrame approach (which would make one line per runway rather than per thresholds), with the full linestring or polygon as a geometry feature could make sense, but then, we should think about distinguishing iteration on runways and iteration on thresholds (and it could be a pain to come back to thresholds from the geometry... wouldn't it?)

Honestly I don't know... So I am not really enthusiast about merging this as is, because even if "of course it wouldn't really hurt", it would be just a "band-aid on a wooden leg" (French colloquialism 🤷)

I would be more than happy to convert that into an issue and think about how to improve that part of the library though!

What do you think?

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