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 PGplot, a method for generating graphs. #966

Draft
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

somiaj
Copy link
Contributor

@somiaj somiaj commented Nov 25, 2023

Here is something I've been working on for a while, but probably best to start getting some feedback.

First, PGplot is a new method for generating dynamic graphs and is meant to be a replacement of WWPlot and PGgraphmacros.pl. The first main difference is PGplot is just an API to store data that can then be used to generate a graph using multiple outputs. Right now only TikZ pgfplots and GD (for testing mostly) is supported. I started trying to fully support both, but as I added more features, I focused only on TikZ pgfplots features (so GD only supports simple things and should probably be removed, but leaving it here for now).

In addition there is a new macro PGplotmacros.pl that is meant to be a drop in replacement of PGgraphmacros.pl. This works in most cases provided the problem doesn't call any GD features directly and doesn't use filling (since I couldn't figure out how to get the GD fill method to work with TikZ). My hope was this could just replace PGgraphmacros.pl, but there are a non trivial number of problems in the OPL that call GD features directly, that this only worked on most of the OPL problems I tested.

Here are a set of examples and testing problems I have been working with to try out. This contains the problems and a few set definition files you can import. PGplot-examples.tar.gz

I am starting this as a draft, as I suspect there are going to be lots of changes as we think about the PGplot API being used and what sort of features it should support.

@somiaj
Copy link
Contributor Author

somiaj commented Nov 25, 2023

Well looks like my method of splittings up in the macros/ directory is causing a lot of tests to fail, since it cannot find the objects that are needed. I will have to look into that.

@somiaj somiaj force-pushed the pgplot branch 3 times, most recently from 6df78c9 to f6081b1 Compare November 29, 2023 04:36
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.

Here are some initial thoughts on this.

The PGplotmacros.pl needs to be renamed to something that does not contain the word "macros".

t/macros/load_macros.t Outdated Show resolved Hide resolved
macros/graph/unionImage.pl Outdated Show resolved Hide resolved
@somiaj
Copy link
Contributor Author

somiaj commented Feb 9, 2024

I removed the PGplotmacros.pl wrapper, and will add that to a future PR. Also combined everything into a single macro file, so now this is just a new macro for creating graphs. The primary output at the time is PGFPlots via a LatexImage, though it is meant to be able to support different outputs.

I'm also noticing a difference in perltidy here and in the tests. In the failed perltidy test, the test wants to add an additional tab to the list of or statements I modified in PGbasicmacros.pl, but perltidy doesn't want that tab here. I'm running perltidy version 20220613 in Debian bookworm, could there be a change there?

@drgrice1
Copy link
Member

drgrice1 commented Feb 9, 2024

Rebase onto develop now, and it will fix the perltidy issue.

@somiaj
Copy link
Contributor Author

somiaj commented Feb 9, 2024

I still have some difference between my local perltidy and the test results. My guess is the older version, so I'll look into updating that.

@drgrice1
Copy link
Member

drgrice1 commented Feb 9, 2024

Make sure you specifically use Perl::Tidy version 20220613. Both newer and older versions will give different results. This is one of my annoyances with Perl::Tidy. When they release new versions, the break compatibility with older versions.

@somiaj
Copy link
Contributor Author

somiaj commented Feb 9, 2024

That is strange, I am using 20220613. I did perltidy PGbasicmacros.pl, and last night when I tried to perltidy that file again, I had no changes. I just tried, and now I have the same changes as the workflow test. Unsure what is going on, but the issue seems to have gone away.

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 think this is a good start. I noted some code issues.

You have several examples in the tar file that you provided that reference a PGplotmacros.pl macro which does not exist. I would like to see that actually work (but not with a PGplotmacros.pl file). That should be the main goal of this. That is to make it so that the code in PGgraphmacros.pl can be eventually replaced with calls to methods here and it will work for existing problems that use that macro. There is the issue of problems using GD objects directly, but I am sure we can find a way to work around that.

I don't like the merge of the previous separate files all into this one file, making it almost 2000 lines long. Instead the PGplot, PGplot::Tikz, PGplot::GD, and PGPlot::Axes should be made actual modules (each a separate file). Then this macro should have just the basics need to instantiate the object (much like how the PGtikz.pl and PGlateximage.pl macros work with respect to the LaTeXImage package). The documentation on how to use the macro should stay here for problem authors (again like the above mentioned macros). Modular code is much better than a single massive file.

macros/graph/PGplot.pl Outdated Show resolved Hide resolved
macros/graph/PGplot.pl Outdated Show resolved Hide resolved
macros/graph/PGplot.pl Outdated Show resolved Hide resolved
macros/graph/PGplot.pl Outdated Show resolved Hide resolved
macros/graph/PGplot.pl Outdated Show resolved Hide resolved
macros/graph/PGplot.pl Outdated Show resolved Hide resolved
macros/graph/PGplot.pl Outdated Show resolved Hide resolved
macros/graph/PGplot.pl Outdated Show resolved Hide resolved
macros/graph/PGplot.pl Outdated Show resolved Hide resolved
macros/graph/PGplot.pl Outdated Show resolved Hide resolved
@somiaj
Copy link
Contributor Author

somiaj commented Apr 9, 2024

@drgrice1 Thanks for the review. @pstaabp has been doing a bit of work on this and has split everything back out and moved this to lib/. He is focused on getting this to work with jsxgraph for html, and also have a way to just export JSON from to plot data. I was informed he may add a PR to my branch with the changes, when those come in I'll implement any of these which are still relevant.

@somiaj
Copy link
Contributor Author

somiaj commented Apr 9, 2024

I removed the PGplotmacros.pl for now because I thought it would be easier to focus on getting this to an acceptable state before dealing with trying to make existing problems work with a wrapper. I was hoping initially to have a drop in replacement, but as I kept running into how many problems were calling GD directly, that I saw this was going to take a lot more work and should probably be dealt with separately. I also think some of my approach, which was working for some problems, was overriding things in an incorrect way, which is what caused it to break all the auto tests.

I do agree it should be a goal that we can just have a drop in replacement and all current problems are updated to using something newer.

@somiaj somiaj force-pushed the pgplot branch 2 times, most recently from be70a50 to 6e500f4 Compare August 15, 2024 02:32
@somiaj
Copy link
Contributor Author

somiaj commented Aug 18, 2024

I updated this, I removed the change of putting everything in a single file, then added @pstaabp change to move everything to lib/, and some code clean up. I also updated all of @drgrice1 suggestions as well.

This move changed the package to just Plots, and the command to create a Plots object to Plot(). Since the Plots object is now apart of lib/, the following patch https://github.com/somiaj/webwork2/tree/pgplot-libs is needed. I have updated all the examples naming.

Plots_examples.tar.gz

In updating this I did notice an issue with pdf2svg and xelatex set as the default. It seems that xelatex isn't fully compatible with pgfplots, though switching this to use dvisvgm by default works just fine (since that calls latex directly). The issue I noticed was with the marks test (Problem 8 of the Test set), the rest worked just fine. I think one of the marks uses a symbol that conflicts with xelatex. I am just documenting this issue for now, the default setup using dvisvgm works properly.

@somiaj somiaj force-pushed the pgplot branch 2 times, most recently from a5ee6fb to b8078d0 Compare November 10, 2024 11:10
@somiaj
Copy link
Contributor Author

somiaj commented Nov 10, 2024

Add the very basics of JSXGraph and Plotly.js output support. Examples updated.

Plots_examples.tar.gz

@somiaj somiaj force-pushed the pgplot branch 3 times, most recently from 98eb31e to 6ccd490 Compare November 10, 2024 13:01
@somiaj
Copy link
Contributor Author

somiaj commented Nov 10, 2024

Thinking about adding JSXGraph and Plotly.js output method a bit more, I think this approach is not the best. Plots is mostly designed around creating non dynamic graphs, so at best adding javascript output could do is allow some moving around/zooming in/out of the graph in some situations.

I think a better approach would create a JSXGraph and Plotly.js plots object that is a subclass, built on the same Plots::Data objects, so it can fall back to using TikZ (or some other non dynamic output) for hard copy, instead of try to make Plots do both non-dynamic and dynamic graphs.

@drgrice1
Copy link
Member

JSXGraph does a great job of rendering static graphs. I use it for that purpose in problems via a local macro that I have.

@somiaj
Copy link
Contributor Author

somiaj commented Nov 10, 2024

I was more thinking about also wanting to be able to also use it for dynamic graphs from the same macro. And unsure if adding the dynamic side of things should be done in Plots, or better to just have a separate macro for that. But so far it is working well with the setup I have here.

@drgrice1
Copy link
Member

Yeah, I think that this is really for static graphs (dynamically generated but not interactive). I mean you could have dynamic in the sense of pan and zoom I suppose (both Plotly and JSXGraph support that).

@somiaj
Copy link
Contributor Author

somiaj commented Nov 10, 2024

Agreed, so my thought was Plotly and JSXGraph could be separate macros that build on top of this, so they can create static and dynamic graphs (as opposed to what I'm working on now which is just making it an optional output method). Though I guess they can split off later.

  PGplot is a new method for generating dynamic graphs and can
  be used as a replacement for WWPlot and PGgraphmacros.pl.
  PGplot is a method to create a plot object and store information
  about a plot in multiple data objects. Since PGplot just stores
  data about the plot, the data can then be used to create multiple
  different outputs. Currently only TikZ pgfplots and GD (for testing)
  are supported.
pstaabp and others added 4 commits December 10, 2024 13:06
This renames the PGplot object to Plots, and moves all the code from
macros/ to lib/. This adds a new macro plots.pl that loads the core
Plots::Plot object via the Plot method.
Mostly unpacking function calls, blessing objects in a single
line, fixing perl calls, along with other code cleanup suggestions.
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