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

Hid non-essential warning messages in time series docs #3890

Merged
merged 3 commits into from
Dec 15, 2022

Conversation

christopherbunn
Copy link
Contributor

@christopherbunn christopherbunn commented Dec 14, 2022

Fixes #3620

A lot of the warning messages that we are seeing has to do with a lot of the internal solvers being unable to find convergence. A significant amount of these errors can be filtered out by filtering them in a hidden cell.

In addition, for the known in advance section, a number of errors are UserErrors related to initial parameter inits in addition to the above convergence error messages. This second cell hiding UserErrors I am a bit more skeptical about as it's a bit broad and leaves room for actual errors to be hidden in the future.

Current page: https://evalml.alteryx.com/en/stable/user_guide/timeseries.html#Known-in-advance-features
Updated page: https://feature-labs-inc-evalml--3890.com.readthedocs.build/en/3890/user_guide/timeseries.html#Known-in-advance-features

@codecov
Copy link

codecov bot commented Dec 14, 2022

Codecov Report

Merging #3890 (e1b9c89) into main (a37d089) will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##            main   #3890   +/-   ##
=====================================
  Coverage   99.7%   99.7%           
=====================================
  Files        346     346           
  Lines      36358   36358           
=====================================
  Hits       36227   36227           
  Misses       131     131           

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

@christopherbunn christopherbunn marked this pull request as ready for review December 14, 2022 19:44
@christopherbunn christopherbunn requested review from eccabay, jeremyliweishih, chukarsten and fjlanasa and removed request for eccabay December 14, 2022 19:44
Copy link
Contributor

@eccabay eccabay left a comment

Choose a reason for hiding this comment

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

Looks good!

Should we file a follow up issue to track removing those parameter not used warnings? It seems like those shouldn't be raised in the first place.

Unrelated to this issue, I'm concerned by the fact that we're getting info-level logging information. We shouldn't be, since verbose is set to false. I think one of our dependencies may be mucking with the logging configuration, which is something for us to keep an eye on.

@@ -7,6 +7,7 @@ Release Notes
* Changes
* Added a threshold to ``DateTimeFormatDataCheck`` to account for too many duplicate or nan values :pr:`3883`
* Documentation Changes
* Hide non-essential warning messages in time series docs :pr:`3890`
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we do past tense 🥲

Copy link
Collaborator

@jeremyliweishih jeremyliweishih left a comment

Choose a reason for hiding this comment

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

@eccabay agreed - filed #3891!

@christopherbunn christopherbunn force-pushed the 3620_hide_timeseries_doc_warnings branch from 5c8f037 to f428d1d Compare December 15, 2022 18:20
@christopherbunn christopherbunn changed the title Hide non-essential warning messages in time series docs Hid non-essential warning messages in time series docs Dec 15, 2022
@christopherbunn christopherbunn enabled auto-merge (squash) December 15, 2022 18:22
@christopherbunn christopherbunn merged commit ffe87af into main Dec 15, 2022
@christopherbunn christopherbunn deleted the 3620_hide_timeseries_doc_warnings branch December 15, 2022 18:51
@christopherbunn christopherbunn mentioned this pull request Jan 3, 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.

Fix the Time Series Docs
3 participants