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

Add tests for histogram plotting #7912

Closed
dakshvar22 opened this issue Feb 9, 2021 · 10 comments · Fixed by #8471
Closed

Add tests for histogram plotting #7912

dakshvar22 opened this issue Feb 9, 2021 · 10 comments · Fixed by #8471
Assignees
Labels
area:rasa-oss/cli Issues focused on the rasa command-line-interface area:rasa-oss 🎡 Anything related to the open source Rasa framework type:enhancement ✨ Additions of new features or changes to existing ones, should be doable in a single PR

Comments

@dakshvar22
Copy link
Contributor

Description of Problem:
Currently there are no tests for histogram plotting inside rasa/utils/plotting.py(def plot_histogram). These should be added, specifically related to the logic of creation of bins.

Overview of the Solution:
Tests for the above function

@dakshvar22 dakshvar22 added type:enhancement ✨ Additions of new features or changes to existing ones, should be doable in a single PR area:rasa-oss 🎡 Anything related to the open source Rasa framework labels Feb 9, 2021
@wochinge wochinge added the area:rasa-oss/cli Issues focused on the rasa command-line-interface label Feb 9, 2021
@ericoBandeira
Copy link
Contributor

Hi, has this issue been solved?

@JEM-Mosig
Copy link
Contributor

@ericoBandeira I was just about to pick it up. Do you want to do it?

@ericoBandeira
Copy link
Contributor

@JEM-Mosig I would like to solve this issue! thank you so much

@JEM-Mosig
Copy link
Contributor

@ericoBandeira Ok, great! Go ahead :) You can assign me to review your PR when you're ready.

@JEM-Mosig JEM-Mosig self-assigned this Mar 9, 2021
@santosm46
Copy link
Contributor

santosm46 commented Mar 11, 2021

@JEM-Mosig Hello! I'm working with @ericoBandeira and I have a question, where should the test be created? as def plot_histogram returns None, I think it has to be at rasa/utils/test.py where there is a function calling plot_curve from rasa.utils.plotting, it returns None such as def plot_histogram.
Also, could you please give more details that can help us to solve this?

@JEM-Mosig
Copy link
Contributor

JEM-Mosig commented Mar 15, 2021

@santosm46 @ericoBandeira The test should be in a new tests/utils/test_plotting.py file. I'd recommend splitting the plot_histogram function into two parts: One (set of) function(s) that computes the bin specification and one function that does the matplotlib stuff. The former part is essentially everything that happens before fig, axes = plt.subplots(...) and is the part that need tests.

@JEM-Mosig
Copy link
Contributor

@santosm46 @ericoBandeira How is this coming along?

@santosm46
Copy link
Contributor

Hi @JEM-Mosig
We're no longer working on this issue, thanks for the attention. Anyone can feel free to work on it.

@JEM-Mosig
Copy link
Contributor

Oh, ok. I'll pick it up then.

@JEM-Mosig
Copy link
Contributor

JEM-Mosig commented Apr 13, 2021

Looking into it, I just notice that the plot is missing some data (cutoffs are wrong). E.g. for:

[[1., 3., 8.], [2., 3., 3.]]

the left histogram only shows two bars, even though it should be three. Will fix this with this issue, too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:rasa-oss/cli Issues focused on the rasa command-line-interface area:rasa-oss 🎡 Anything related to the open source Rasa framework type:enhancement ✨ Additions of new features or changes to existing ones, should be doable in a single PR
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants