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

Fixed issue in list_plot where it assumed data had been enumerated when it might not have been #39481

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

Noel-Roemmele
Copy link
Contributor

Fixes #29960. Fixes issue where data was assumed to be enumerated but there are cases where it may not be. If the data was a list of complex numbers it was previously assumed that the data at the very end was guaranteed to be enumerated as it would enumerate the data in a previous catch block. However if the first element of the list is not a real number the list is not enumerated as the catch block fails before it enumerates anything. This was allowing weird errors as seen in the original bug fix request.

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

@DaveWitteMorris
Copy link
Member

Thanks for the fix. The code already passed CI testing, except that Lint found problems with the formatting (and the documentation failed to build, perhaps because of the Lint error). Here are minor comments that I think will fix that (and more).

Standard coding style in python is for variables to be all-lowercase (unless a word, such as someone's name, needs to be capitalized), with an underscore between words. (Mixed case is used for class names.) So you should change listEnumerated to list_enumerated.

In line 3111, you need to change :trac: to :issue:. (This is the error found by Lint.)

I think the #-comments could be clarified. (These are tests, not examples, and I think it may not be clear what "Non enumerated" means, unless the reader has a good understanding of the code.) I suggest "Test the codepath where ``list_enumerated`` is ``False``." and "Test the codepath where ``list_enumerated`` is ``True``."

Sagemath does not normally use #-commented lines in doctests. Instead, ReST syntax should be used to show that it is a comment. (Reduce the indentation, add two colons at the end, and add a blank line after it.)

    Test the codepath where ``list_enumerated`` is ``False``::

        sage: list_plot([3+I, 4, I, 1+5*i, None, 1+i])
        Graphics object consisting of 1 graphics primitive

    Test the codepath where ``list_enumerated`` is ``True``::

        sage: list_plot([4, 3+I, I, 1+5*i, None, 1+i])
        Graphics object consisting of 1 graphics primitive

Copy link

Documentation preview for this PR (built with commit 33316d1; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@DaveWitteMorris
Copy link
Member

CI is happy with this, and so am I. Will set to positive review on Thursday if there are no other comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

list_plot plots y-values on x-axis when a value is None
2 participants