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

Generic Tests for CappingTransformer #200

Merged
merged 4 commits into from
Apr 15, 2024
Merged

Conversation

davidhopkinson26
Copy link
Contributor

@davidhopkinson26 davidhopkinson26 commented Mar 21, 2024

Adding Generic tests for Capping Transformer.

Realised that the sole test in ColumnsFromDictInitTests is actually specific to Mapping Transformers so have moved this to BAseMappingTransformer tests. Creating a similar test for Capping Transformers to Inherit.

@davidhopkinson26 davidhopkinson26 changed the title Feature/generic tests capping Generic Tests for CappingTransformer Mar 21, 2024
@limlam96
Copy link
Contributor

limlam96 commented Apr 8, 2024

Picked this one up to avoid going stale as David is on AL.

Did a bit of a refactor of the capping file to make it fit better with the new testing, main change is pulling out shared functionality into 'BaseCapping' class. In the process had to edit OutOfRangeNullTransformer, so scope did creep to updating tests for that one too (sorry!)

Did highlight that there are weight checks that can probably be moved into BaseTransformer, or another higher level class. But will leave for another issue.

@limlam96 limlam96 marked this pull request as ready for review April 8, 2024 13:01
Copy link

@biggyfatty biggyfatty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall looks good
saw some of tests script has OtherBaseBehaviourTests, some not, any specific reason (don't think OtherBaseBehaviourTests has anything in it tho)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not something for this pr: if we have a chance, would it be better to reorder minimal_attribute_dict to alphabetic order (or anything makes it easier to navigate?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re OtherBaseBehaviourTests, which ones have you spotted that are missing it? think we should be adding

And sure will set up an issue re min attribute dict :)

@limlam96 limlam96 merged commit 7b8f03e into main Apr 15, 2024
12 checks passed
@limlam96 limlam96 deleted the feature/generic_tests_capping branch April 15, 2024 14:29
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.

4 participants