⚠ This page is served via a proxy. Original site: https://github.com
This service does not collect credentials or authentication data.
Skip to content

Conversation

@pstaabp
Copy link
Member

@pstaabp pstaabp commented Jan 30, 2026

This creates a StatisticalPlots macro, which contains the methods add_barplot, add_histogram, add_boxplot and add_scatterplot.

There are many options for each and there is documentation as well.

This also includes an add_rectangle method to the plots.pl macro which is a wrapper for the add_dataset for creating rectangles.

This is all in a single perl file, but can split out into modules like the plots macro.

I'm making this a draft and there are some additional options I'd like to add. I'm also open to renaming items and adding other plots before this goes in.

Here's a few test problems to show how to use it:

stats.zip

Note: updated zip file.

This contains the methods add_barplot, add_histogram, add_boxplot and
add_scatterplot.

There are many options for each and there is documentation as well.

This also includes the add_rectangle method to the plot.pl macro
which is a wrapper for the add_dataset for creating rectangles.
@somiaj
Copy link
Contributor

somiaj commented Jan 31, 2026

I am a little confused, is the lib/Plots/StatPlot.pm an initial attempt, before making this fully a macro? Seems the same object and methods (which are more robust) are all included in macros/graph/StatisticalPlot.pl, which is probably a better place, and the file in lib/Plots should be removed?

Copy link
Member

@drgrice1 drgrice1 left a comment

Choose a reason for hiding this comment

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

I haven't delved to deeply into this yet, but here are some initial observations.

Lines 100 and 105 are too long. Shorten them. Ideally the POD would be kept even shorter than the usual 120 character limit. I think a max of 100 characters is a better limit for that, maybe even the classical 80 character limit would be better. The POD is all textual and if you are trying to read it directly in the code it makes it easier to read if it is not so long. Generally, you have kept the lines to 100 characters or less, but there are a few others that are inconsistently longer.

I think you should eliminate the StatPlot.pm module entirely. There is not much code there, and so just put that directly into the StatisticalPlots.pl macro file. The macro file is not that large anyway (at least at this point compared to many others). The plots.pl macro is bigger, and it only has 5 lines of actual code!

next;
}
if (ref $image_item eq 'Plots::Plot') {
if (ref $image_item eq 'Plots::Plot' || ref $image_item eq 'Plots::StatPlot') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be worthwhile to make this more general, for future extension macros. Maybe call your package Plots::Plot::StatPlot, and then have this only check if ref $image_item starts with Plots::Plot, that way this won't have to be updated each time someone wants to extend Plots?

Copy link
Contributor

Choose a reason for hiding this comment

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

I also think there are some other places that might need to be updated, I recall having to modify a few places to check for the ref being equal to Plots::Plot beyond just this place (for some older image macros).

Copy link
Member

Choose a reason for hiding this comment

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

Ideally this would be an isa call. Then any object that derives from a Plots::Plot object would work here. Unfortunately, to properly do that you need the Scalar::Util::blessed method which is not available here. The correct way to check if an object, say $object, derives from a particular class is if (blessed($object) && $object->isa('Parent::Package')).

When I was creating the SimpleGraph.pl macro, I almost made that package derive from the Plots::Plot package, and then wanted to make this code that way, but ran into the issue with the lack of the blessed method availability. I ended up going a different direction with the SimpleGraph.pl macro though.

Copy link
Member

Choose a reason for hiding this comment

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

Also, line 2946 below needs to check the ref also.

Copy link
Member

Choose a reason for hiding this comment

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

DO NOT add this ref check to either macros/core/VectorField2D.pl or macros/graph/unionImage.pl. There are checks for the Plots::Plot object in those macros.

The VectorField2D.pl macro usage doesn't make sense for this statistical graph macro.

As to the unionImage.pl one, I told @somiaj not to add that there. It should be removed. No one should be using that macro anymore, and it should be moved into the deprecated folder.

use strict;
use warnings;

use WeBWorK::Utils qw(min max);
Copy link
Member

Choose a reason for hiding this comment

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

I just noticed this. This definitely can't be. A PG package can not use a WeBWorK package or the methods from it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think those should be in the PGstatisticsmacros.pl macro--I think there are functions in there that use a variation of that--I have some changes to that macro. Perhaps I'll move min/max into there as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nevermind, this is in PGauxillaryFunctions.pl, and I use these.

@drgrice1
Copy link
Member

OSX users need to learn to not include the __MACOSX directory in the archive files that they distribute!

Copy link
Member

@drgrice1 drgrice1 left a comment

Choose a reason for hiding this comment

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

I see now that this pull request doesn't even use the lib/Plots/StatPlot.pm file. So just delete it, and ignore my comments in that file.

@drgrice1
Copy link
Member

Here are some issues seen with your example problems:

The histogram.pg example in your zip file is not working. It gives the warning Use of uninitialized value $options{"orientation"} in string eq at line 358 of [PG]/macros/graph/StatisticalPlots.pl. and the resulting graph looks quite bad.

The boxplot_vertical.pl problem does not show a vertical box plot. In fact the problem is identical to the boxplot.pl file. Even adding the orientation => 'vertical' option to the add_boxplot call does not make it so.

The scatter01.pg file is the new problem template, and not a scatter plot at all.

@pstaabp
Copy link
Member Author

pstaabp commented Jan 31, 2026

About the StatPlot.pm file. I didn't mean to commit that. Sorry I notice that it made it in.

Where's the __MACOS directory? MacOS has this .DSStore file, but that's not in here either.

About scatter01.pg, I forgot to save changes. Here's an update of those problems--I just added one--with some new functionality.

stats.zip

Cleanup of the POD.
Removal of Plots::StatsPlot->new function.  It wasn't needed.
Make sure Plots::StatsPlot objects are rendered in PGbasicmacros.pl.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants