Automasted testing for plots from ControlSystems

[freemin7]

I was encouraged to find a solution to automate testing for the plotting in ControlSystems.jl

We don't have automatic testing for plots, so it would be great if you could test making some plots with multiple systems, where subsequent systems have phase curves that span both tighter and wider intervals

I have thought about the best way to achieve this. The only way i could come up with is committing pictures of plots to this package which seems a bit dubious to me. (You should commit binaries into git)
This approach also carries the human cost that the plots (and the package) might need to be rerun after every update.
Can i have your thoughts on that?

[baggepinnen]

Testing plotting automatically is very hard. Updates to the plot packages might cause very small changes in the plots by, e.g., shifting margins, axes etc. by a few points. Being robust against these small changes while detecting meaningful differences between plots is a hard problem that is still an active research area. Simply doing sum(abs2,plot_new - plot_old) is hopelessly fragile and will produce a very large error if say, a vertical black line against a white background is shifted horizontally by a single pixel.

I can imagine that an approach based on optimal transport would handle this very well, but it still remains troublesome. Optimal transport is computationally expensive, and threshold values are still required to be set somehow.

[mfalt]

We are using VisualRegressionTests for comparison here, but I am not sure how well it is doing. I think a nice solution would be that this package monitors pull requests to ControlSystems.jl, runs the tests and posts the status with links to comparisons on the original pull request automatically. I don't know if this is possible with for example the github actions.

[mfalt]

I have thought about the best way to achieve this. The only way i could come up with is committing pictures of plots to this package which seems a bit dubious to me. (You should commit binaries into git)

This should already happen when running this package, if it still works. The way it used to work was that it ran the set of examples and compared the images to the saved ones. If there was a significant difference, the package would ask if the reference should be replaced with the new one.

[baggepinnen]

I don't know if this is possible with for example the github actions.
Unfortunately not (yet) in a push fashion, but could be doable by periodic polling.

[mfalt]

I have been looking into it a bit, and it seems like it should be possible as an action on the ControlSystems.jl repo. For example, adding a trigger:

on:
  pull_request:
    types: [opened, reopened, edited]

works fine, see my run on mfalt/ControlSystems.jl#2
Changing the action to run a script that does the testing should be trivial. I wrote one such script that seems to work, and automatically posts a comment on a PR.
The things that are lacking so far is

• connecting the script to the action, and uploading the images somehow. The script I wrote creates a new branch, named e.g, tmp-plots-7102543e-6fb2-11ea-098b-3d902f6744aa, to which it pushes the old and new figures.
• pushing the changes (i was able to create and commit all the files through the julia script)
• ensuring that everything is secure
• making sure that the workflow is not blocking a pull

The juliascript would look something like this https://gist.github.com/mfalt/58e9ad8bb38041cd98801e6ae65d8188

[mfalt]

If its is possible to add a on: pull_request on this repo that looks at ControlSystems.jl, then this would probably be optimal. This would require that a workflow on this repo is allowed to add a comment to ControlSystems.jl, which I assume is possible since they share organization. It should then also be possible to have a workflow that updates the reference images once a PR is pulled onto master on ControlSystems.jl

[mfalt]

Ok, this seems entirely doable, I registered a bot that can get access to ControlExamplePlots, and I am able to send a message from ControlSystems.jl that triggers an action on ControlExamplePlots.jl, and verify that is is sent by the bot. The bot would then only need access to post comments on ControlSystems.jl. I will see if i can finalize this over the weekend. What do you think @baggepinnen ?

[baggepinnen]

Sounds like it's worth a try. I was working on a transport-based distance between images earlier today but did not have time to finish it. Could potentially be useful to make distance computations more robust. In the meantime, there's also the package https://github.com/JuliaImages/ImageQualityIndexes.jl

[mfalt]

I have it up and running now! (on my own fork to not mess something up). The result can be seen here (i had to close and open once because I had a bug in the bot):
mfalt/ControlSystems.jl#3
The workflow is defined here on the sending side:
https://github.com/mfalt/ControlSystems.jl/blob/master/.github/workflows/PingControlExamplePlots.yml
and on the receiving side:
https://github.com/mfalt/ControlExamplePlots.jl/blob/master/.github/workflows/PrPingReciever.yml
And the scrip that is being run is here:
https://github.com/mfalt/ControlExamplePlots.jl/blob/master/utils/pr_commenter.jl
Any comments are welcome.

Edit: The bot only has access to ControlExamplePlots.jl as a collaborator. I guess this could be circumvented by making it push to its own fork. But I think the security is pretty good. It works by having a secret token for the bot stored in the settings for ControlExamplePlots.jl repo.

[mfalt]

Closing since we have implemented this now. Thanks for helping us out with ideas. If you have any suggestions on how to improve the current implementation, feel free to comment on #8