Skip to content
New issue

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

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

Already on GitHub? # to your account

Update _topics.py #2254

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

ajdapretnar
Copy link

Proposed solution to not having enough topics and the call visualize_topics() resulting in an Error.

What does this PR do?

Fixes #2247.
Not sure whether this requires updating documentation or a test. If yes, happy to write it.

Before submitting

Proposed solution to not having enough topics and the call visualize_topics() resulting in an Error.
Copy link
Owner

@MaartenGr MaartenGr left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! I left a couple of small questions/comments.

Comment on lines +60 to +62
elif len(topics) < 5:
warnings.warn("Too few topics to display.")
return
Copy link
Owner

Choose a reason for hiding this comment

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

Couple of questions:

  • What makes you choose the value of < 5 here? Is it working for 4 topics but it is for 5?
  • Should we raise an error rather than return nothing after a warning? By returning None, we might also have to change the typing.
  • Could you extend the description a bit more? Perhaps explain that we need more topics in order to properly visualize them?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issue Warning when plotting MDS with two few topics
2 participants