Skip to content

& yarnabrina [MNT] move deep_equals and dependency checkers from testing to utilities to remove accidental coupling to pytest#178

Merged
fkiraly merged 15 commits into
mainfrom
move-deep_equals
May 13, 2023
Merged

& yarnabrina [MNT] move deep_equals and dependency checkers from testing to utilities to remove accidental coupling to pytest#178
fkiraly merged 15 commits into
mainfrom
move-deep_equals

Conversation

@fkiraly
Copy link
Copy Markdown
Contributor

@fkiraly fkiraly commented May 4, 2023

This PR moves deep_equals and dependency checkers from testing to utilities to remove accidental coupling to pytest.

Also see here for the problem caused by this: #177 - this fixes #177

Crediting @yarnabrina for the fix as suggested in sktime/sktime#4544 (comment)

@fkiraly fkiraly changed the title [MNT] move deep_equals and dependency checkers from testing to utilities to remove accidental coupling to pytest & yarnabrina [MNT] move deep_equals and dependency checkers from testing to utilities to remove accidental coupling to pytest May 4, 2023
Copy link
Copy Markdown
Member

@yarnabrina yarnabrina left a comment

Choose a reason for hiding this comment

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

I know almost nothing about how skbase works, but is it not necessary to add deep_equals to __init__.py? Or, it'd be this way during deprecation period, and then changed?

return x


def _pandas_equals(x, y, return_msg=False):

Check notice

Code scanning / CodeQL

Explicit returns mixed with implicit (fall through) returns

Mixing implicit and explicit returns may indicate an error as implicit returns always return None.
@fkiraly
Copy link
Copy Markdown
Contributor Author

fkiraly commented May 4, 2023

I know almost nothing about how skbase works, but is it not necessary to add deep_equals to init.py? Or, it'd be this way during deprecation period, and then changed?

Unfortunately the module name is the same as the function name, so it would result in a clash to import the function to the same module that contains the module of the same name, no?

Might be suboptimal...

@yarnabrina
Copy link
Copy Markdown
Member

yarnabrina commented May 4, 2023

Unfortunately the module name is the same as the function name, so it would result in a clash to import the function to the same module that contains the module of the same name, no?

Just to be clear, I meant doing something like this in skbase/utils/__init__.py:

## existing imports

from sktime.utils.deep_equals import deep_equals

## or if you accept relative imports
## from .deep_equals import deep_equals

## existing codes

Tried locally, and it works.

>>> 
>>> from skbase.utils import deep_equals as a
>>> from skbase.testing.utils.deep_equals import deep_equals as b
/path/to/skbase/fork/skbase/testing/utils/_dependencies.py:10: UserWarning: _check_soft_dependencies, _check_python_versiontesting have moved to skbase.utils.dependencies. The old location will be removed in skbase 0.6.0.
  warn(
/path/to/skbase/fork/skbase/testing/utils/deep_equals.py:10: UserWarning: deep_equals has moved to skbase.utils.deep_equals. The old location will be removed in skbase 0.6.0.
  warn(
>>> 
>>> a
<function deep_equals at 0x7f680574b040>
>>> b
<function deep_equals at 0x7f680574b040>
>>> 
>>> b is a
True
>>> 

(/path/to/skbase/fork is obvious edited, but it works without any clash.)

Can you please check #181?

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 4, 2023

Codecov Report

Merging #178 (ca7fcd4) into main (9a5c70c) will decrease coverage by 0.16%.
The diff coverage is 64.36%.

📣 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     #178      +/-   ##
==========================================
- Coverage   82.89%   82.74%   -0.16%     
==========================================
  Files          39       43       +4     
  Lines        2777     2805      +28     
==========================================
+ Hits         2302     2321      +19     
- Misses        475      484       +9     
Impacted Files Coverage Δ
skbase/utils/dependencies/_dependencies.py 57.42% <57.42%> (ø)
skbase/utils/deep_equals.py 64.96% <64.96%> (ø)
skbase/base/_base.py 80.06% <100.00%> (ø)
skbase/testing/test_all_objects.py 79.35% <100.00%> (ø)
skbase/testing/utils/_dependencies.py 100.00% <100.00%> (+42.57%) ⬆️
skbase/testing/utils/deep_equals.py 100.00% <100.00%> (+30.46%) ⬆️
skbase/testing/utils/tests/test_deep_equals.py 100.00% <100.00%> (ø)
skbase/utils/dependencies/__init__.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

@fkiraly fkiraly merged commit 790ab90 into main May 13, 2023
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.

[BUG] clone breaks when skbase is installed without pytest

4 participants