[adjusters/foca] Exclude (by name) Skyguide features from FOCA dataset#30
[adjusters/foca] Exclude (by name) Skyguide features from FOCA dataset#30
Conversation
| EXCLUDED_FEATURES_NAMES = ["CTR DUEBENDORF", "CTR ZURICH"] | ||
|
|
||
|
|
||
| def _exclude_feature(f: Feature) -> bool: |
There was a problem hiding this comment.
nit (take it or leave it): I read this as a little clearer on whether we're asking ("exclude this feature?" or telling ("exclude this feature and tell me if successful or not"):
| def _exclude_feature(f: Feature) -> bool: | |
| def _should_exclude_feature(f: Feature) -> bool: |
| 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 |
2 participants
Fix focagis/geoawareness-cis#12
I've opted to set in the adjuster the excluded features by name as 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.