[BUG] fix faulty BaseObject.__eq__ and deep_equals if an attribute or nested structure contains np.nan

[fkiraly]

BaseObject.__eq__ and deep_equals return the wrong answer if somewhere in the structure an np.nan occurs. This is because np.nan != np.nan by default.

For BaseObject-s, this can lead to failures of equality whenever somehwere in the nested structure (of parameters) an np.nan is contained.

This happens to be the case for tuner default parameter settings in sktime, which makes the refactor fail for those specific estimators, see here: sktime/sktime#3151 (comment)

This bugfix handles the case np.nan in deep_equals, which fixes it in its reverse dependency BaseObject.__eq__.

Curiously, the np.nan case was handled in sktime, but incorrectly. The check in sktime incorrectly considers any two floats as being equal (rather than just two instances of np.nan which happens to be of type float).
In another failure, the handling of np.nan was removed (instead of being fixed) when deep_equals was ported to skbase.

Together, this explains the presence of the bug in skbase, the failure of tests in the refactor PR on sktime tuners, and the absence of test failures on the main branch of sktime.

I've also added more test cases for deep_equals to cover the failures around np.nan which are being fixed here.

[fkiraly]

@RNKuhns, why are unrelated tests (for package structure lookup) failing here?

[fkiraly]

I get it, you used skbase itself for testing and hard coded expected retrieval. Not sure whether that is such a good idea? Opened issue here to discuss: #114

[codecov-commenter]

Codecov Report

Merging #111 (f38782e) into main (86e0c4e) will increase coverage by 0.02%.
The diff coverage is 88.88%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #111      +/-   ##
==========================================
+ Coverage   78.05%   78.07%   +0.02%     
==========================================
  Files          23       23              
  Lines        1877     1884       +7     
==========================================
+ Hits         1465     1471       +6     
- Misses        412      413       +1     

Impacted Files	Coverage Δ
skbase/testing/utils/tests/test_deep_equals.py	100.00% <ø> (ø)
skbase/testing/utils/deep_equals.py	64.90% <88.88%> (+1.01%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[RNKuhns]

@fkiraly the all_extras optional dependencies in pyproject.toml needs to be updated to include numpy.

This doesn't fail in our CI workflow, because that is installing all test dependencies and numpy is already a test dependency. But we want to give users the easy of installing the extras.

[RNKuhns]

I get it, you used skbase itself for testing and hard coded expected retrieval. Not sure whether that is such a good idea? Opened issue here to discuss: #114

Yea -- it expedient at the time. I've commented on #114 on way forward on that.

[RNKuhns]

@fkiraly the other thing I didn't mention explicitly in my review is that we need to determine how you want this fix published. If having it available by early-February in sktime refactor is fine with you then we can proceed as usual with merging issues.

However, if you want a patch (bug fix) release, we want to keep the changes there to minimum (bug fixes only) if possible (which impacts decision on merging other branches). Let me know what you are thinking on the timing.

Moving forward, it might make sense to open a release branch and merge PRs for a release into that branch. That way it is easy to do quick bug fix releases, since we know main will have limited changes.

[fkiraly]

@fkiraly the all_extras optional dependencies in pyproject.toml needs to be updated to include numpy.

Is that really an omission or problem of this PR?
Feels more like a separate PR, since the status quo was using numpy already in the same way (same dependency pattern).

[fkiraly]

@fkiraly the other thing I didn't mention explicitly in my review is that we need to determine how you want this fix published. If having it available by early-February in sktime refactor is fine with you then we can proceed as usual with merging issues.

I just overrode __eq__ in sktime, we should just make sure we don't forget to revert that. So there's no hard blocker through this, and no need for an urgent timing, imo.

[fkiraly]

Yea -- it expedient at the time. I've commented on #114 on way forward on that.

I don't see any comments on #114, did they get lost? Sometimes I push the comment button and internet disconnects, and it does not get posted.

[RNKuhns]

Yea -- it expedient at the time. I've commented on #114 on way forward on that.

I don't see any comments on #114, did they get lost? Sometimes I push the comment button and internet disconnects, and it does not get posted.

@fkiraly it appears they did. But the gist of it was that we should make a PR that expands the mock_package in the test module, so that it covers more cases needed for tests (non-public functions, modules to make sure retrieval can exclude them; non-public modules to make sure we can exclude those and some sub-module nesting, etc). I'll try to do that this week.

[fkiraly]

just to clarify, do you want the numpy dep in this PR? I don't mind either way, just thought it might be cleaner separately.
(in the file change history, it would show up with a PR/commit description that seems completely unrelated, which is going to be confusing for sth as substantial as a dep change - for context, I typically copy the PR description as the squash commit merge message for good documentation of the edit history)

[RNKuhns]

I'm fine with it being separate PR. Can you open that?

[fkiraly]

Sure, opened a PR with numpy here: #121

[RNKuhns]

@fkiraly wanted to verify this is good to go (with all updated logic from sktime) before merging.

[fkiraly]

yes, good to go from my side.

[fkiraly]

(you still haven't approved? is there sth I should change?)

[RNKuhns]

LGTM. Adding of numpy soft dependency is in #121.