-
Notifications
You must be signed in to change notification settings - Fork 47
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
Visualization: Fix legend
argument checking for waterfall/parameter/history plots
#1139
Conversation
Co-Authored-By: Dilan Pathirana <dilan.pathirana@uni-bonn.de>
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. @@ Coverage Diff @@
## develop #1139 +/- ##
===========================================
+ Coverage 83.06% 84.37% +1.30%
===========================================
Files 148 148
Lines 11642 11649 +7
===========================================
+ Hits 9671 9829 +158
+ Misses 1971 1820 -151
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for noticing, looks good aside from some slight changes .
@PaulJonasJost what do you think of the suggestion for raising different errors for legend type and length, also in the case of a single result? |
Co-authored-by: Paul Jonas Jost <70631928+PaulJonasJost@users.noreply.github.com>
Co-authored-by: Paul Jonas Jost <70631928+PaulJonasJost@users.noreply.github.com>
ready to go, @plakrisenko |
@@ -80,18 +84,19 @@ def process_result_list( | |||
legends.append('Result ' + str(i_leg)) | |||
else: | |||
# legends were passed by user: check length | |||
if isinstance(legends, list): | |||
try: | |||
if isinstance(legends, str): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this is true it already means that 'List of results passed and list of labels do not have the same length.', right? As it's a multiple result case + only a string was passed as a legend
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The string could have matching length, e.g. "ABC" for 3 results.
The idea was that a string of length != len(results) would throw an error, but a string with the appropriate number of characters would be used for the legend. We could also raise an error for any string, but this would not be consistent with the type hint for the legend argument of visualize.waterfall: legends: Optional[Union[Sequence[str], str]] = None, because a string is a Sequence.
legend
argument checking for waterfall/parameter/history plots
legend_error
would be thrown when the user supplied the legends as a tupleTypeError
for single and multiple resultsValueError
in case of multiple results when lengths of results and legends do not match