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

Boxplot Options #7655

Closed
wants to merge 5 commits into from
Closed

Boxplot Options #7655

wants to merge 5 commits into from

Conversation

Fidel365
Copy link
Contributor

Fixes(partially) #7556
Procurement>Describe>Numeric>Boxplot
@N-thony , this PR is ready for review

@Fidel365 Fidel365 added this to In progress in Developers via automation Jul 26, 2022
@Fidel365
Copy link
Contributor Author

@N-thony , the recent commit fixes minor overlapping issues

@lloyddewit
Copy link
Contributor

@rdstern please could you test? thanks

@rdstern
Copy link
Collaborator

rdstern commented Jul 30, 2022

@Fidel365 I note this (and some other of your pull requests seem to overlap with those by @EstherNjeriLiberatta . For this one - boxplots - how do your changes relate to her pull request in #7570 ?

There are others that seem to overlap too. For example you have #7656 and @EstherNjeriLiberatta had #7572?

@Fidel365
Copy link
Contributor Author

Fidel365 commented Aug 1, 2022

@rdstern , we are working on the same dialogues but from different menus, we are trying to update all dialogues to geom options.e.g The boxplot commit follows the path Climatic>Describe>Boxplot

@rdstern
Copy link
Collaborator

rdstern commented Aug 1, 2022

@Fidel365 thank you for the clarification. It was just that at the top of your pull request, you mention the Procurement menu. The boxplot dialogue there is just the same as the main one. That's unlike the Climatic > Check Data > Boxplot that you have now mentioned and that is using a different dialogue - so great that you are adapting that one. I'll check it now.

@N-thony
Copy link
Collaborator

N-thony commented Aug 29, 2022

@EstherNjeriLiberatta can you test/review this? Thanks.

Developers automation moved this from In progress to Review in progress Sep 9, 2022
instat/dlgBoxPlot.vb Outdated Show resolved Hide resolved
instat/dlgBoxPlot.vb Outdated Show resolved Hide resolved
instat/dlgBoxPlot.vb Outdated Show resolved Hide resolved
instat/dlgBoxPlot.vb Outdated Show resolved Hide resolved
instat/dlgBoxPlot.vb Outdated Show resolved Hide resolved
instat/dlgBoxPlot.vb Outdated Show resolved Hide resolved
instat/dlgBoxPlot.vb Outdated Show resolved Hide resolved
instat/dlgBoxPlot.vb Outdated Show resolved Hide resolved
@N-thony
Copy link
Collaborator

N-thony commented Sep 27, 2022

@derekagorhom can you take it over?

@derekagorhom
Copy link
Contributor

@derekagorhom can you take it over?

@Fidel365 still working on it, i will help him with the corrections

@Fidel365
Copy link
Contributor Author

@EstherNjeriLiberatta , this PR is ready for a rereview

@lloyddewit
Copy link
Contributor

lloyddewit commented Oct 3, 2022

@EstherNjeriLiberatta , this PR is ready for a rereview

@EstherNjeriLiberatta Thank you for your review and comments above. I checked the new commit and all your comments above are resolved.
I understand that you're currently working on other projects. To avoid potential delays, I'm happy to do the next peer review of this PR.

@Fidel365 Thank you for your work on this PR. Is this PR ready for testing or are you still working on this?

Thanks

@Fidel365
Copy link
Contributor Author

Fidel365 commented Oct 3, 2022

@lloyddewit , it's ready for testing

@lloyddewit
Copy link
Contributor

@rdstern Please can you test? thanks

Copy link
Collaborator

@rdstern rdstern left a comment

Choose a reason for hiding this comment

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

@N-thony I thought we should re-allocate this work. I have many comments below.
But I have been working backwards in the pull requests and this seems to be duplicating #7570?
If so, then this can be closed, because the other is much better. If you agree, then simply close this one.

  1. Please note that the dialogue has now got out-of line - compare the position of the receivers for the Points and the Group option in 0.7.7 and the new one. I give both below:
    image
    And in the new one:
    image
    The Add Points and Groups to Connect should move back to their original positions.
    b) Change Box to Boxplot
    c) Add the other options (Tufte, Jitter, Violin) into the pull-down. Note that the work has been, because when I move to Jitter or Tufte or Violin and press on Box it goes to the right place.
    d) I don't mind if they all remain available all the time. But if you are disabling those that don't apply, then note that you should enable Jitter, (as well as Boxplot) when you Add Points.
    e) Also add Summary when you join the means/medians and go to stat_summary.?

@rdstern
Copy link
Collaborator

rdstern commented Nov 20, 2022

@N-thony after the above I am closing this one now, and continuing with #7570 .
Please reopen if I have got that wrong.

@rdstern rdstern closed this Nov 20, 2022
Developers automation moved this from Review in progress to Done Nov 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants