Bring MeanResponseTransformer in line with new test set up #262
Conversation
… some tests still failing due to tests in base_tests.y not giving a y argument to fit
…tests still failing so need to investigate
…ged affected tests in base_tests.GenericFirTests and GenericTransformTests to refer to df[a] for target
…amed to TestPriorRegularisation
|
fyi CI is failing, but happy to approve otherwise! |
Bit strange, an error seems to have appeared with NearestMeanResponseImputer. Will investigate. |
…esponseImputer.fit()
…mn 'b' by default
|
Had to reconfigure a few things in Imputer tests to make some shared generic tests compatible with this transformer.
I'm wondering if it might be best to separate out GenericFitTests for the limited subset of cases where y is a compulsory argument (this and NearestMeanResponse imputer). Quite a few of the generic tests before this PR did not specify a y argument. I think it is better that they do even though most can take y=None (and in fact default to it) as they will mostly be called in a pipeline where y will be specified so we need to assert they work in those circumstances. Arguably a test checking those which can accept None work in those circumstances should be created. |
|
Hi David - picking up this review as Liam is on AL. On your comment regarding transformers where y is not a compulsory argument and a test to confirm they work when None is passed should be created, is there a ticket for this? If not can we get one down :) |
|
Reviewed scripts, no further comments on Liam's additional review. Agree with the changes made to standardise things across the imputer test scripts |
|
Bringing MeanResponseTransformer in line with the new testing set up.
Have noticed during this update that WeightColumnFitMixinTests and WeightColumnInitMixinTests might not be being used in other transformers we have already refactored so might be worth a quick audit.