[adjusters/foca] Exclude (by name) Skyguide features from FOCA dataset#30
[adjusters/foca] Exclude (by name) Skyguide features from FOCA dataset#30
Conversation
| if "properties" not in f or f.properties is None: | ||
| return False | ||
| if "name" not in f.properties or f.properties.name is None: | ||
| return False | ||
|
|
||
| for name in f.properties.name: | ||
| if "text" not in name or name.text is None: | ||
| return False | ||
| if name.text in EXCLUDED_FEATURES_NAMES: | ||
| return True |
There was a problem hiding this comment.
If other Features could be excluded for other reasons in the future, I would structure this to more easily allow that (see below). If this function is only ever going be used to exclude features by name, I would narrow the function name to reveal that choice (e.g., _feature_has_excluded_name or just _has_excluded_name).
| if "properties" not in f or f.properties is None: | |
| return False | |
| if "name" not in f.properties or f.properties.name is None: | |
| return False | |
| for name in f.properties.name: | |
| if "text" not in name or name.text is None: | |
| return False | |
| if name.text in EXCLUDED_FEATURES_NAMES: | |
| return True | |
| if "properties" in f and f.properties and "name" in f.properties and f.properties.name: | |
| for name in f.properties.name: | |
| if "text" in name and name.text in EXCLUDED_FEATURES_NAMES: | |
| return True |
| for za in f.properties.zoneAuthority: | ||
| za.purpose = _role_for(original_type) | ||
|
|
||
| # filter out features whose names are specified in EXCLUDED_FEATURES_NAMES |
There was a problem hiding this comment.
With just my InterUSS hat on and naively reading the Issue, it seems like the exclusion might want to additionally or alternatively match on identifier (CTR0004, CTR0015)?
Either way, I would suggest not describing how _exclude_feature makes its decisions as that creates a brittle coupling between documentation and implementation:
| # filter out features whose names are specified in EXCLUDED_FEATURES_NAMES | |
| # filter out features that are intentionally excluded |
There was a problem hiding this comment.
For background, the initial intention was to avoid duplicates between Skyguide and FOCA datasets by filtering out from the FOCA dataset the features that exist already in the Skyguide one, matching them on the name since the identifier is different (which is why in this PR I've opted for matching on the name).
And because deduplicating by comparing the two files does not seem to make sense within the context of this tool, I've opted for just listing the features to exclude.
But indeed the end result is not ideal. Matching on identifier and making this an additional parameter in the config (rather than part of the adjuster) seems more sensible, allow re-use, and enable easier changes in the future. I will modify this PR in consequence.
|
@BenjaminPelletier could you review the PR again? As I've changed it substantially. Thanks |
2 participants
Fix focagis/geoawareness-cis#12
I've opted to add a configuration option
excluded_features_ed318_identifiersas I did not find a sensible way of parsing the Skyguide dataset. Please LMK if that's not OK and we can discuss how to do that.