Feature extractors #38
Conversation
…o-gfmap into feature_extractors Conflicts: src/openeo_gfmap/fetching/s1.py src/openeo_gfmap/preprocessing/interpolation.py src/openeo_gfmap/preprocessing/udf_rank.py src/openeo_gfmap/preprocessing/udf_score.py src/openeo_gfmap/utils/catalogue.py src/openeo_gfmap/utils/intervals.py tests/test_openeo_gfmap/test_cloud_masking.py
|
|
||
| REQUIRED_IMPORTS = """ | ||
| import inspect | ||
| from abc import ABC, abstractmethod |
There was a problem hiding this comment.
why are inspect and abc required imports?
There was a problem hiding this comment.
inspect is useless in this case, I removed it, but ABC is used as it defines the base class for all feature extractors
There was a problem hiding this comment.
I'm actually wondering if you can't eliminate the whole REQUIRED_IMPORTS by replacing those inspect.getsource(...) constructs with just concatenating the python files that define these classes, then all your dependencies are handled automatically and you don't have to manually keep a REQUIRED_IMPORTS in sync
There was a problem hiding this comment.
Moreover, I think it's useful to have unit tests on that UDF source code building pipeline, e.g. check that the generated UDF code can at least be parsed (e.g. to just parse with executing with ast.parse from ast module or full compile with compile() built-in), and ideally also check that it can be loaded (e.g. all imports are valid, ...)
| udf_code += f"{REQUIRED_IMPORTS}\n\n" | ||
| udf_code += f"{inspect.getsource(FeatureExtractor)}\n\n" | ||
| udf_code += f"{inspect.getsource(PatchFeatureExtractor)}\n\n" | ||
| udf_code += f"{inspect.getsource(feature_extractor_class)}\n\n" |
There was a problem hiding this comment.
FYI: idea for future improvement: instead of manually putting the source of FeatureExtractor, PatchFeatureExtractor, feature_extractor_class here, you could also walk the inheritance trail automatically
|
General note: my review feedback doesn't necessarily has to block merging this PR already. This is quite a big PR and keeping it open too long is just asking for conflicts with other feature branches I'm fine with merging this as-is and fixing things up in follow-up branches/PRs |
Created feature extractors for users. A class must be created by extending the
PatchFeatureExtractorclass and implementing two methods:executeandget_labels. Theexecutemethod contains the logic applied in the UDF, while the methodget_labelsspecifies the names of the feature bands to be applied after the UDF in the openeo job.