ENH Add inverse_transform feature to SimpleImputer - #17612

Could you parametrize the test with different missing_values type.

Testing for two missing_values, [-1, np.nan]

I would try 2 more arrays: one where all columns have missing data and one where there is missing data one column every two

Thanks for spotting this!
I overlooked a major scenario where features get dropped for being completely empty in the transform step. The process of regenerating original data is not very elegant when columns are dropped resulting in a different shape. I think changes suggested by PR #16695 would make things easier here.
The logic I wrote right now is iterative. It works only when both the fitting data and transform data are compatible i.e., both have values missing in same features. Let me know if there's a better way to handle this limitation.

what I mean was more

    X_3 = np.array([
        [1, missing_value, 5, 9],
        [missing_value, 4, missing_value, missing_value],
        [2, missing_value, 7, missing_value],
        [missing_value, 3, missing_value, 8]
    ])

    X_4 = np.array([
        [1, 1, 1, 3],
        [missing_value, 2, missing_value, 1],
        [2, 3, 3, 4],
        [missing_value, 4, missing_value, 2]
    ])

Thank you. I update the test cases

I would call it X_original

no need for the comment

What is the difference with what you called feature_count originally?

feature_count is the count of features that not completely empty. And orig_cols is the original number of features (including empty features). Changed their names to make it clearer.

ok I see

It is weird to use ptr because we don't have pointers actually. Do you mean col for column.

you can use idx if we deal with indices thought.

Yes, I actually mean column indices. Changed the names now.

Iterate over X_1 ... X_4 in a for loop

The test case for X_2 relies on X_1, so kept them separate. Added a loop over X_3 and X_4.

I don't see the difference. You create an imputer, transform, inverse_transform, and check the original X with the twice transformed array no?

I agree with @glemaitre that leaving these two out of the loop looks inexplicable to the reader.

X_2 is different in the sense that we only perform transform on it using the imputer that is fit on X_1. For X_3 and X_4, we do a fresh fit_transform. The purpose of X_2 was to test how the imputer would perform on data that is not used for fitting.

You should put this in a separate test

Moved this into a separate test

I would match for Got 'add_indicator={self.add_indicator}' to be sure that we replace by the expected value

what I mean was more

    X_3 = np.array([
        [1, missing_value, 5, 9],
        [missing_value, 4, missing_value, missing_value],
        [2, missing_value, 7, missing_value],
        [missing_value, 3, missing_value, 8]
    ])

    X_4 = np.array([
        [1, 1, 1, 3],
        [missing_value, 2, missing_value, 1],
        [2, 3, 3, 4],
        [missing_value, 4, missing_value, 2]
    ])

use original_idx

orig is not the right name -> inv_trans instead

inv_trans

move these line outside from the raises. It will be more explicit to know which line is raising the error

I would match for Got 'add_indicator={self.add_indicator}' to be sure that we replace by the expected value

I agree with @glemaitre that leaving these two out of the loop looks inexplicable to the reader.