Can you please add a test (like just copy the one in#24545)?

(This seems to be the correct fix, although my knowledge of masked is very limited...)

Nice detective work@chahak13! Could you add a test that would have failed without this change? Maybe adapt the example from#24545 into animage test.

[Edit: cross-post with@oscargus, but it is good to see we agreed!]

Btw, the same thing cannot happen here as well?

But maybe paths cannot have units or masked arrays to start with?

It may be that this is actually better solved inset_offsets:

which would then remove the need for an additional unit conversion etc.Although one probably should move the logic ofget_offsets forNone values toset_offsets and callset_offsets from the constructor. (Have not checked all the consequences and that doesn't really have to be part of this PR...) Edit: no, but either setself._offset in constructor in case of None or callset_offsets if not None. But not sure if it is a good idea as it is not clear when masked is wanted or not as you point out@chahak13 . But it seems a bit inconsistent anyway.

Anyway, probably better to get this in and then spend some effort trying to make it more unified. The risk is that it will take quite a bit of time to untangle everything...

@oscargus good point. This might actually be a more deep-rooted issue. I checkedPath and it does take in masked arrays (which it then converts to NaNs) so that should also use masked array functions. Also, as you pointed out, this is probably something that should be set directly inset_offsets. Not all functions use the masked versions, maybe it is worthwhile to put in effort to recognize and change those?

I've added a test and baseline images for this for now. This should resolve the current issue but I'll try to dig into the usage of masked data later. Let me know if there's something missing!

Thanks!

Maybe, for the sake of it, add a@check_figures_equal test where the same is used for one of the figures and the other masks the x-data instead?

(It may be enough to only have one of the format, e.g., png, but that would require a rebase with force push as the cleanliness test will become unhappy otherwise.)

Oh, and if there is a way to actually get a masked Path with units to test with that would be great, but shouldn't be a show stopper for now.

@oscargus@rcomer, need advice. I was working on the@check_figures_equal test and it wasn't working correctly if I masked the datetime array instead ofy. On further digging, this goes into the maintenance issue that Oscar just opened.

TLDR:date2num converts the masked data array to a normal numpy array which is why the test was failing. What do you suggest I should do? I can't write the test to compare the two cases of masking without this.

The reason the fix that I made worked for this case is that the mask was on a numeric field. For all the non-number types,_is_natively_supported() returnsFalse and then asks the axis' converter to convert the data. The data contains the mask information until this point, so every converter in the registry should be able to handle the masked array by either using masked array functions or raising a warning about it to ensure that the user doesn't expect the masked information in the plot.

I'd say skip the equal test then for now. It may be a deep rabbit hole...

I'll add a comment to#24733 pointing this out. Probably the right thing is to support masked arrays indate2num, but that may lead to other strange things...

Understood, thanks. I'll resolve the 2 comments on the review then.

That should do it. Let me know if there's anything needed!

Thanks! FYI, it wasn't that tricky to get masked dates to pass:#24734

But we can add the image equal test when digging into the other aspects later.

Yes, it was pretty straight forward! I had the fix on my local machine but just wasn't sure if we wanted to include that in this or would that be out of scope for this particular PR. Thanks though!

Ahh, sorry about that then. Just didn't want you to go down a potential rabbit hole by encouraging it...