Skip to content

Make blank plot instead of raising on all nan#584

Merged
nvaytet merged 5 commits into
mainfrom
do-not-raise-on-all-nan
Jul 2, 2026
Merged

Make blank plot instead of raising on all nan#584
nvaytet merged 5 commits into
mainfrom
do-not-raise-on-all-nan

Conversation

@nvaytet

@nvaytet nvaytet commented Jul 1, 2026

Copy link
Copy Markdown
Member

Sometimes you have a slice of data that contains all NaNs, but you still want to inspect the rest of the data with e.g. the slicer.

Example:

import plopp as pp
%matplotlib widget

da = pp.data.data3d()
da['z', 0].values[...] = np.nan
pp.slicer(da)

This would raise with a message saying it cannot compute limits because data is all NaN.
Now we instead return a NaN range, which get converted to a null range [0, 0].
The plot is just blank instead. Moving the slider can still be used to view other healthy slices.

@nvaytet nvaytet requested a review from jokasimr July 2, 2026 13:04
values = fix_empty_range(find_limits(x, scale=scale, pad=pad))
values = find_limits(x, scale=scale, pad=pad)
if np.isnan(values[0].value) or np.isnan(values[1].value):
return dict.fromkeys(keys, None)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If this is really meant to be treated equivalently to the case that raises ValueError then it could make sense to raise ValueError here too instead of returning the dict, so that we avoid duplicating that logic.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think here we want to make a bbox with None, so that it gets ignored.

Say you plot 2 lines, and one of them has all NaN. I would expect to just see one line instead of having an error raised. You might then wonder why you have just one line, and in that case raising an error may be safer. But in the case of interactive plots, where one slice somewhere may have all nans and the others are fine, I think it makes sense to not show the data instead of raising.

@jokasimr jokasimr Jul 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I didn't mean to raise an error that is visible to the user. I meant that the ValueError will be catched by the surrounding try/catch, and then we return the same dict.fromkeys anyway but with less duplication.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

So what you meant was:

        values = find_limits(x, scale=scale, pad=pad)
        if np.isnan(values[0].value) or np.isnan(values[1].value):
            raise ValueError("NaN limits")

?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yes

@nvaytet nvaytet enabled auto-merge July 2, 2026 14:01
@nvaytet nvaytet merged commit 07f8db8 into main Jul 2, 2026
5 checks passed
@nvaytet nvaytet deleted the do-not-raise-on-all-nan branch July 2, 2026 14:07
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.

2 participants