Skip to content

[ENH] sync TestAllObjects with sktime#139

Merged
RNKuhns merged 4 commits into
mainfrom
test-update
Mar 10, 2023
Merged

[ENH] sync TestAllObjects with sktime#139
RNKuhns merged 4 commits into
mainfrom
test-update

Conversation

@fkiraly
Copy link
Copy Markdown
Contributor

@fkiraly fkiraly commented Mar 4, 2023

This syncs TestAllObjects with the current sktime state:

  • added docstrings
  • some descriptive error messages
  • sharper logic in testing the output of clone in test_clone

@fkiraly fkiraly added the implementing framework Implementing core skbase framework label Mar 4, 2023
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 4, 2023

Codecov Report

Merging #139 (6dcbe04) into main (39f6adf) will decrease coverage by 0.01%.
The diff coverage is 85.71%.

📣 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     #139      +/-   ##
==========================================
- Coverage   82.68%   82.68%   -0.01%     
==========================================
  Files          32       32              
  Lines        2322     2327       +5     
==========================================
+ Hits         1920     1924       +4     
- Misses        402      403       +1     
Impacted Files Coverage Δ
skbase/testing/test_all_objects.py 78.98% <85.71%> (+0.01%) ⬆️

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

Also tests inheritance and super call logic in the constructor.

Tests that:
* create_test_instance results in an instance of estimator_class
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 don't think this is going to render correctly in sphinx. I think that bulleted lists need a space between the list bullets and the text above or below

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.

So in this case you'd need something like:

"""Some doc summary.

Some extended summary.

Tests that:

* first bullet
* second bullet
* third bullet
"""

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

resolved in all instances

# assert deep_equals(object_clone.get_params(), object_instance.get_params())
"""Check that clone method does not raise exceptions and results in a clone.

A clone of an object x is an object that:
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.

same as above.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

same, resolved


Tests that:
* constructor has no varargs
* tests that constructor constructs an instance of the class
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.

Same as above.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

same, resolved

Copy link
Copy Markdown
Contributor

@RNKuhns RNKuhns left a comment

Choose a reason for hiding this comment

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

@fkiraly some quick docstring suggestions.

For some reason our read-the-docs integration that builds the docs as part of PRs doesn't seem to be working. I'll try to get that back up and running.

Might be worth building the docs locally and taking a look at how these render.

@fkiraly
Copy link
Copy Markdown
Contributor Author

fkiraly commented Mar 8, 2023

For some reason our read-the-docs integration that builds the docs as part of PRs doesn't seem to be working. I'll try to get that back up and running.

I don't quite get how readthedocs integration works to modify contents of the PR. I thought there was an automatic doc build, and readthedocs then gets the latest version from the main branch? But readthedocs doesn't actually trigger the automatic doc build.

I might be mistaken, so I would appreciate some explanation to understand what you mean.

@fkiraly
Copy link
Copy Markdown
Contributor Author

fkiraly commented Mar 8, 2023

formatting stuff is resolved as far as I can see.

@fkiraly fkiraly requested a review from RNKuhns March 8, 2023 18:20
@RNKuhns
Copy link
Copy Markdown
Contributor

RNKuhns commented Mar 10, 2023

For some reason our read-the-docs integration that builds the docs as part of PRs doesn't seem to be working. I'll try to get that back up and running.

I don't quite get how readthedocs integration works to modify contents of the PR. I thought there was an automatic doc build, and readthedocs then gets the latest version from the main branch? But readthedocs doesn't actually trigger the automatic doc build.

I might be mistaken, so I would appreciate some explanation to understand what you mean.

It doesn't modify the PR. But we do have ReadTheDocs setup to build the docs for each PR. So it is the PR that triggers RTD to build the docs as one of the checks we run on PRs. I know I'm a stickler for documentation -- but I've been using it to make sure things are rendering like we'd want.

For some reason the RTD build of the PR docs wasn't working for about a day. But it is working now. So I'll take a look and wrap this up.

@RNKuhns
Copy link
Copy Markdown
Contributor

RNKuhns commented Mar 10, 2023

@fkiraly the lists are rendering better. There are a few extra things that aren't rendering exactly like we'd want. But I won't hold this up for that now. We'll take another pass across the skbase.testing documentation ahead of upcoming presentation.

Copy link
Copy Markdown
Contributor

@RNKuhns RNKuhns left a comment

Choose a reason for hiding this comment

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

I'm approving, since we plan to take another pass at the testing module documentation ahead of presentation.

We'll catch the remaining minor docstring formatting items at that time.

@RNKuhns RNKuhns merged commit b662e28 into main Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

implementing framework Implementing core skbase framework

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants