fix(preprocessing): remove constant NaN columns#1061
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the RemoveConstantFeaturesStep to correctly identify and drop columns that consist entirely of NaN values for both PyTorch and NumPy inputs, and adds corresponding unit tests. The review feedback points out a potential runtime error when applying NaN checks on integer-type inputs, suggesting that we should verify if the input has a floating-point data type before checking for NaNs.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
One thing to consider is that #1055 will pass |
adrian-prior
left a comment
There was a problem hiding this comment.
Generally, lgtm! I am still wondering about what you have mentioned:
Do inf/-inf not (at least theoretically) carry more signal than just all NaNs? E.g., I might have sensors that record inf/-inf if the values fall outside the range I can measure. If this is the case, we would drop columns that could carry meaningful signal.
I am not super in the loop on the current inf/NaN, so apologies if this doesn't make sense.
|
Good question!
I get your point with sensors, but I'm not sure how often we'll see the same sensor only ever output |
for now this isn't an issue because we don't drop all-NaN columns, but when #1061 gets merged, it'd drop columns with a mix of inf, -inf, NaN because of the passthrough implementation
adrian-prior
left a comment
There was a problem hiding this comment.
Sorry, for coming back late I was OOO on Friday.
Approving this to not block anything, but I am not sure that I am convinced yet!
I get your point with sensors, but I'm not sure how often we'll see the same sensor only ever output inf and -inf in the same dataset in practice; sounds like a very faulty sensor, but I guess that is information too!
If we don't think -inf/+inf should be treated differently from NaNs, then shouldn't we just overwrite them as NaNs in a previous step and check only for NaNs here? I feel like I might be missing some details regarding what the exact idea of this refactor is, but I feel like either (a) we treat Infs as a first class citizen or (b) just overwrite them with NaNs. I don't really see how the in-between makes sense.
|
Thanks! |
Issue
Closes PRI-241 — all-NaN columns were not being removed as constant columns, affecting downstream preprocessing.
Motivation and Context
RemoveConstantFeaturesStepdecides which columns to keep using an equality comparison (X[0:1, :] == X). For an all-NaN column every comparison isFalse(NaN != NaN), so the column was treated as non-constant and kept, even though it carries no information. These columns then propagate NaNs into later preprocessing steps.This adds an explicit guard so a column that is entirely NaN is also treated as constant and dropped:
~np.all(np.isnan(X), axis=0)is AND-ed into the keep mask.& ~X.isnan().all(dim=0).At this point in the pipeline
Xis always a numeric float array (categoricals/strings are ordinally encoded and cast tofloat64byclean_dataupstream), sonp.isnan/Tensor.isnanare safe.Public API Changes
How Has This Been Tested?
Checklist
changelog/README.md), or "no changelog needed" label requested.