[GSProcessing] Add support for Rank-Gauss Feature Transformation #615
Conversation
thvasilo
added
the
gsprocessing
There was a problem hiding this comment.
Looks good overall, just need some small changes.
Did we perform a small integration test to ensure the config value get propagated as expected all the way to the transformation from the a GConstruct/GSProcessing config? The unit tests do not cover that AFAIK.
graphstorm-processing/graphstorm_processing/config/config_conversion/gconstruct_converter.py
Outdated
Show resolved
Hide resolved
...hstorm_processing/data_transformations/dist_transformations/dist_numerical_transformation.py
Outdated
Show resolved
Hide resolved
...hstorm_processing/data_transformations/dist_transformations/dist_numerical_transformation.py
Outdated
Show resolved
Hide resolved
...hstorm_processing/data_transformations/dist_transformations/dist_numerical_transformation.py
Outdated
Show resolved
Hide resolved
...hstorm_processing/data_transformations/dist_transformations/dist_numerical_transformation.py
Outdated
Show resolved
Hide resolved
graphstorm-processing/tests/test_dist_numerical_transformation.py
Outdated
Show resolved
Hide resolved
Actually that is what I do first. I always first to make a GConstruct/GSProcessing config convert/run correctly, and then start to write unit-test and e2e test. I did not add that here is that we do not have a GSProcessing e2e test. Maybe it will be necessary in the future. For feature transformation, I think the single unit tests will cover most of the cases. But at least for my PRs, I always do some local e2e test first. |
...hstorm_processing/data_transformations/dist_transformations/dist_numerical_transformation.py
Outdated
Show resolved
Hide resolved
…ns/dist_transformations/dist_numerical_transformation.py Co-authored-by: Theodore Vasiloudis <[email protected]>
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.