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

Plugin Docs: Python Tools & Matplotlib #2726

Merged

Conversation

codingS3b
Copy link
Contributor

@codingS3b codingS3b commented Oct 15, 2018

Adds sections on how to develop and use the new python tools.

It already includes documentation for the jupyter_widgets as well (see #2691)

@ax3l ax3l self-requested a review October 15, 2018 11:11
@ax3l ax3l self-assigned this Oct 15, 2018
@ax3l ax3l added component: tools scripts, python libs and CMake documentation regarding documentation or wiki discussions labels Oct 15, 2018
Copy link
Member

@ax3l ax3l left a comment

Choose a reason for hiding this comment

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

Thank you very much!
For the PR, I suggest little open improvements and have a few follow-up PR tasks we should tackle next.

Python postprocessing tool structure
====================================

For each plugin there are (or have to be implemented) three different classes.
Copy link
Member

Choose a reason for hiding this comment

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

Each plugin should implement at least one of the following Python classes.

2. A visualizer class that outputs a matplotlib plot
3. A jupyter-widget class that exposes the parameters of the matplotlib visualizer to the user
via other widgets

This comment was marked as resolved.


Data reader
~~~~~~~~~~~
The data readers should reside in the ``lib/python/picongpu/plugins/`` directory.
Copy link
Member

Choose a reason for hiding this comment

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

This is a follow-up to this PR:
they should actually get their own sub-directory soon for clean-up :)
E.g., lib/python/picongpu/plugins/data

def __init__(self, run_directory)

def get_data_path(self, your_specific_arguments)
# has to return the path to the underlying data file
Copy link
Member

Choose a reason for hiding this comment

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

Can we write the usual

"""
Comments as now...

Returns
-------
str
    ...
"""
...

format?

The visualizers should reside in the ``lib/python/picongpu/plugins/plot_mpl/`` directory.
The module names should end on ``_visualizer.py`` and the class name should only be ``Visualizer``.

There is a base class for visualization found in ``base_visualizer.py`` which already handles the plotting logic. It uses the
Copy link
Member

Choose a reason for hiding this comment

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

As in latex, just start a newline when starting a new sentence. No limit on line length in prose text to break for.

This will make diffs more readable.

Python postprocessing
=====================

In order to further work with the data produced by a plugin during a simulation run, PIConGPU provides
Copy link
Member

Choose a reason for hiding this comment

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

please keep one sentence per line for smaller diffs.



They can be found under ``lib/python/picongpu/``.
It is our goal to provide three modules for each plugin to make postprocessing as convenient as possible:
Copy link
Member

Choose a reason for hiding this comment

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

provide at least

the thing is: every plugin can have multiple data readers and multiple data visualization methods. For example, the energy histogram can analyze

  • a histogram
  • an integrated charge per opening angle in a detector for a range in the histogram
  • cut-off energies
  • ...

@@ -6,7 +6,7 @@ PNG
This plugin generates **images in the png format** for slices through the simulated volume.
It allows to draw a **species density** together with electric, magnetic and/or current field values.
The exact field values, their coloring and their normalization can be set using ``*.param`` files.
It is a very rudimentary and useful tool to get a first impression on what happens in the simulation and to verify that the parameter set chosen leads to the desired physics.
It is a very rudimentary and useful tool to get a first impression on what happens in the simulation and to verify that the parameter set chosen leads to the desired physics.
Copy link
Member

Choose a reason for hiding this comment

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

Just spotted a double whitespace between "to" and "the desired physics".


.. note::

For a 2D simulation, even a 2D image can be a quite heavy output.
Make sure to reduce the preview size!

It is possible to draw the borders between the GPUs used as white lines.
It is possible to draw the borders between the GPUs used as white lines.
Copy link
Member

Choose a reason for hiding this comment

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

Just spotted a double whitespace between "It" and "is possible to ...".


Each reader class needs to implement the following interface functions:

.. code:: python
Copy link
Member

Choose a reason for hiding this comment

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

Did we forget to write a base class for this? If so, we should build one in a follow-up PR.

Copy link
Member

Choose a reason for hiding this comment

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

Another follow-up to this PR:
On Friday, we realized we forgot to add a meta-data method to return which units (and unit dimensions) the read data have. We should add a get_meta or similar method. We should make this similar or identical to unitSI and unitDimension in openPMD and/or can rely on pint-strings.

Copy link
Contributor Author

@codingS3b codingS3b Oct 16, 2018

Choose a reason for hiding this comment

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

Yes, so far we don't have a base class for the readers. Would you want it to contain all the interface functions (and raising NotImplementedError or should this class already implement some logic?
I think only the phase_space.py file contains some metadata class. Is it something like this you had in mind?

Copy link
Member

Choose a reason for hiding this comment

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

Implemented in #2730

Copy link
Member

Choose a reason for hiding this comment

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

added now with last commit to avoid updating the interface in the manual and the source (now source only :) )

@ax3l ax3l added this to the 0.5.0 / 1.0.0: Next Stable milestone Oct 15, 2018
@codingS3b
Copy link
Contributor Author

codingS3b commented Oct 16, 2018

I adressed most of your comments locally but after rebasing on the current dev, I am not able to generate my docs anymore, the make html fails with Exception occurred: File "/usr/local/lib/python2.7/dist-packages/sphinx/domains/cpp.py", line 4060, in _add_symbols assert len(withDecl) <= 1 AssertionError .
Did I do something wrong?

@ax3l
Copy link
Member

ax3l commented Oct 16, 2018

That's a Sphinx 1.8 bug. You need to downgrade to sphinx 1.7: #2725
It's already fixed in Sphinx master and maybe an upcoming sphinx 1.8.2 or 1.9 release will ship the fix.

@codingS3b
Copy link
Contributor Author

codingS3b commented Oct 16, 2018

Actually I'm using Sphinx 1.7.0. Anyway, it is working now...don't know what the issue was.

@codingS3b codingS3b force-pushed the documentation_python_vis branch from 91918eb to 3be055c Compare October 16, 2018 14:12
@codingS3b
Copy link
Contributor Author

codingS3b commented Oct 16, 2018

What do I have to change so that the py_postprocessing.rst also shows up in the navigation on the left of the page in the development section?

@ax3l ax3l mentioned this pull request Oct 16, 2018
21 tasks
Allow Numpy-style Python autodocs for classes, modules and members
with Napoleon.
@ax3l
Copy link
Member

ax3l commented Oct 17, 2018

Don't be confused: I moved all widget docs to a follow-up in #2691.
The rationale behind this is that the doc PR here shall still go into 0.4.0, but the widgets go into 0.5.0/1.0.0 since they are still in an early draft.

@ax3l ax3l force-pushed the documentation_python_vis branch from 3c159ba to c1e2cd2 Compare October 17, 2018 00:12
@ax3l ax3l changed the title Documentation of python visualizers Plugin Docs: Python Tools & Matplotlib Oct 17, 2018
@ax3l ax3l added the component: plugin in PIConGPU plugin label Oct 17, 2018
@ax3l ax3l force-pushed the documentation_python_vis branch from c1e2cd2 to 16d9a6a Compare October 17, 2018 00:20
ax3l
ax3l previously approved these changes Oct 17, 2018
Add section on postproc tools (PNG, Energy Histogram, PS).
Devel: Architecture of postprocessing tools.
@ax3l ax3l force-pushed the documentation_python_vis branch from 16d9a6a to 3832df5 Compare October 17, 2018 00:25
@ax3l
Copy link
Member

ax3l commented Oct 17, 2018

@codingS3b are you ok with my modifications and can I merge this? :)

@codingS3b
Copy link
Contributor Author

Yes, thanks for restructuring!

@ax3l ax3l merged commit 400e8d3 into ComputationalRadiationPhysics:dev Oct 17, 2018
@ax3l ax3l mentioned this pull request Oct 17, 2018
@ax3l ax3l mentioned this pull request Oct 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: plugin in PIConGPU plugin component: tools scripts, python libs and CMake documentation regarding documentation or wiki discussions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants