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 matplotlib an optional dependency #287

Merged
merged 15 commits into from
Jan 1, 2020
Merged

Conversation

kinnala
Copy link
Owner

@kinnala kinnala commented Dec 24, 2019

Starting work on #286 .

@kinnala
Copy link
Owner Author

kinnala commented Dec 24, 2019

@gdmcbain can you make a quick review at this point? Comments on the overall structure would be appreciated and if you have any ideas or alternative implementation strategies.

I'll continue with fixing rest of the examples after you agree this is the right way to go forwards.

Copy link
Contributor

@gdmcbain gdmcbain left a comment

Choose a reason for hiding this comment

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

  • Should skfem.visualize itself be a package like skfem.importers with skfem.visualize.matplotlib underneath it like skfem.importers.meshio? This might facilitate later adding other visualization backends like yt, pyvista, holoviews, …. Matplotlib is obviously primary for its ubiquity and good enough for most one- and two-dimensional work but it distorts three-dimensional views.

  • Instead of the chains of if (isinstance, ...) would abc or functools.singledispatch be tidier?

@gdmcbain
Copy link
Contributor

Another thought for the further future: Thus far the visualization is based on the skfem.Mesh class hierarchy but it also partially implies ElementLineP1 for MeshLine, ElementTriP1 for MeshTri, etc., or sometimes P0, Q0, etc., I think. Some visualization backends (Gmsh, ParaView?, ...) might support higher order elements. Does eventually supporting higher order visualization imply anything about the structure of the skfem.visualization package? Should the functions take bases rather than meshes? (Noting that an skfem.assembly.GlobalBasis has a .mesh attribute anyway.)

Removes the following features:
- Drawing of piecewise constant functions for Mesh2D
- Drawing of piecewise-linear DG functions for MeshTri

These should be implemented using InteriorBasis.refinterp instead.
@kinnala
Copy link
Owner Author

kinnala commented Dec 29, 2019

Fantastic suggestions, as usual. Here is a notebook I used during development.
plotting.pdf

I'm going to implement also the possibility of drawing through InteriorBasis since I removed the plotting of piecewise-constant and DG functions and refinterp should be used instead. I was thinking syntax like this:

plot(interior_basis, solution)

and the call is the passed to interior_basis.refinterp and, e.g., plot(m: MeshTri, ...).

@kinnala
Copy link
Owner Author

kinnala commented Dec 29, 2019

Next I start fixing the examples. It might take a while.

@kinnala
Copy link
Owner Author

kinnala commented Dec 30, 2019

Starting by running the Makefile under docs/examples and fixing all errors. Later fixing the docs.

@kinnala
Copy link
Owner Author

kinnala commented Dec 31, 2019

Starting to be ready for review. Fixes #286 .

@kinnala kinnala changed the title WIP: Make matplotlib an optional dependency Make matplotlib an optional dependency Dec 31, 2019
@kinnala
Copy link
Owner Author

kinnala commented Dec 31, 2019

Am I missing some examples?

@kinnala kinnala mentioned this pull request Jan 1, 2020
@kinnala kinnala merged commit ce6b641 into master Jan 1, 2020
@gdmcbain gdmcbain deleted the matplotlib-to-optional-dep branch January 1, 2020 22:08
@gdmcbain gdmcbain mentioned this pull request Aug 25, 2021
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