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

Make Plotting optional and extendible #313

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

jtravs
Copy link
Contributor

@jtravs jtravs commented Feb 2, 2023

Use PythonPlot instead of PyPlot, to try and solve mac crash issues
This is WIP because it seems PythonPlot keeps Python 0-based indexing rather than converting to 1-based.

@jtravs jtravs marked this pull request as draft February 2, 2023 16:47
@chrisbrahms
Copy link
Collaborator

Does this actually fix the crash issues?

@jtravs
Copy link
Contributor Author

jtravs commented Feb 2, 2023

Does this actually fix the crash issues?

Not sure yet, but given that PyPlot is essentially unmaintained in favour of PythonPlot, this makes sense anyway. I will test the crash issues this evening. I think I have this working now.

@chrisbrahms
Copy link
Collaborator

That’s reasonable. Thanks for sorting this. I suppose we should really have a test script to make sure all the plotting functions run without error

@jtravs
Copy link
Contributor Author

jtravs commented Feb 2, 2023

This just got a lot harder than I expected. Anywhere there is indexing this messes up.

@jtravs
Copy link
Contributor Author

jtravs commented Feb 2, 2023

I need to leave this for now. For my future reference, this currently errors on a time_1d plot for a multimode simulation, on line Plotting.jl:637. Basically, we call Maths.fwhm on the data from the plot itself.

@jtravs jtravs marked this pull request as ready for review February 2, 2023 21:14
@jtravs
Copy link
Contributor Author

jtravs commented Feb 2, 2023

I believe this is complete now. AFAICT all Plotting functions work with both single and multimode output. I didn;t check all of the examples. Early indications are that this doesn't fix the crashing on mac.

@chrisbrahms
Copy link
Collaborator

This PR seems to be required now. It fixes the issue with installing conda from scratch on Ubuntu, which has existed for some people since at least March (see e.g. https://discourse.julialang.org/t/conda-not-installing-matplotlib-for-pyplot/96813/3). This is now getting more important as it prevents the new release of Luna to be auto-merged.

Couple of small issues to be fixed still, working on those now.

@jtravs jtravs changed the title WIP: use PythonPlot Make Plotting optional and extendible Nov 8, 2024
@jtravs
Copy link
Contributor Author

jtravs commented Nov 8, 2024

@chrisbrahms this has been significantly overhauled. It now uses Julia extensions to enable optional code loading. This has two big benefits:

  1. The code can be run without requiring Python or other plotting packages, e.g. on HPC systems
  2. We can try different plotting backends

Currently, I have implemented PythonPlot, PyPlot and Makie. They all seem to work fine. I would suggest that we recommend PythonPlot as it is the supported matplotlib interface. Personally I am using Makie more as it crashes less on mac.

Still to do:

  • Remove gratuitously duplicated code in the backend files
  • Test much more
  • Check examples and documentation for consistency

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.

2 participants