Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Fix TimeSince check of inconsistency between time_index and start #4015

Merged
merged 3 commits into from Dec 29, 2022

Conversation

KishManani
Copy link
Contributor

@KishManani KishManani commented Dec 29, 2022

What does this implement/fix? Explain your changes.

There is a check that the type of the time index and the start argument are consistent. The check was still being done on X.index which is fine for series data, but not for panel or hierarchical data. The check has been changed to the time_index variable.

PR checklist

For all contributions
  • I've added unit tests and made sure they pass locally.
  • The PR title starts with either [ENH], [MNT], [DOC], or [BUG] indicating whether the PR topic is related to enhancement, maintenance, documentation, or bug.

@KishManani KishManani changed the title [BUG] Fix "TimeSince" check of inconsistency between time_index and… [BUG] Fix TimeSince check of inconsistency between time_index and… Dec 29, 2022
@KishManani KishManani changed the title [BUG] Fix TimeSince check of inconsistency between time_index and… [BUG] Fix TimeSince check of inconsistency between time_index and start Dec 29, 2022
fkiraly
fkiraly previously approved these changes Dec 29, 2022
Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

Good catch!

@aiwalter
Copy link
Contributor

@KishManani as you are on it, can you kindly fix the Examples section, its not rendering well:
https://www.sktime.org/en/latest/api_reference/auto_generated/sktime.transformations.series.time_since.TimeSince.html?highlight=TimeSince

The comments basically have to be indented

@KishManani
Copy link
Contributor Author

KishManani commented Dec 29, 2022

Thanks for spotting that @aiwalter! I've made the change to the examples. But the parameters also don't look like they're rendering how they should.

image

I checked out the docs on documentation and the examples of good documentation. I can't see an example with a bullet pointed list in the parameters section. If you know of any examples or how I should change the docstring so that the parameters section renders correctly please let me know!

Making the docs locally by running make html takes a very long time. Do you know if there is a quicker way for me to check if my docstrings are being rendered correctly? Thank you!

@aiwalter
Copy link
Contributor

You can check Imputer. The CI/CD has a build of the PR docs. You can click on it once passed

@fkiraly
Copy link
Collaborator

fkiraly commented Dec 29, 2022

I checked out the docs on documentation and the examples of good documentation. I can't see an example with a bullet pointed list in the parameters section.

Then there should be one - let's open an issue for someone to link one (and perhaps add explanation what can be learnt from the examples on the documentation page).

Might be best if you do this, @KishManani, since you know best what you were looking for and didn't find, so you can best specify what the documentation page should have done/explained?

@KishManani
Copy link
Contributor Author

Might be best if you do this, @KishManani, since you know best what you were looking for and didn't find, so you can best specify what the documentation page should have done/explained?

Sure! Will do.

@KishManani
Copy link
Contributor Author

You can check Imputer. The CI/CD has a build of the PR docs. You can click on it once passed

Thanks @aiwalter, this is very handy indeed! Do you happen to know of a faster workflow? It can take up to 5 minutes or longer for this to run. The workflow for me is currently like the following: 1) look at documentation and spot errors, 2) take a guess as to whether I need to change indentation or some other fix, 3) render the docs and check if the fix worked. I was wondering if you know of a quicker way to do step 3) 😅 ?

For some reason the formatting is not consistent in my list in the parameters section. For some reason "time-like" is bolded.
image

Any suggestions?

@fkiraly
Copy link
Collaborator

fkiraly commented Dec 29, 2022

Any suggestions?

Get an exorcist

Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

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

Good for me.

I'd say, the doc formatting issue is different scope and hence a different PR.
Let's merge this so there isn't too much conflict with PR #4018

@fkiraly fkiraly merged commit b74bfb6 into sktime:main Dec 29, 2022
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.

None yet

3 participants