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

Update plot_wigner_sphere and delete matrix_histogram_complex #2193

Merged
merged 37 commits into from Jul 19, 2023

Conversation

tamakoshi2001
Copy link
Contributor

@tamakoshi2001 tamakoshi2001 commented Jul 10, 2023

Checklist
Thank you for contributing to QuTiP! Please make sure you have finished the following tasks before opening the PR.

  • Please read Contributing to QuTiP Development
  • Contributions to qutip should follow the pep8 style.
    You can use pycodestyle to check your code automatically
  • Please add tests to cover your changes if applicable.
  • If the behavior of the code has changed or new feature has been added, please also update the documentation in the doc folder, and the notebook. Feel free to ask if you are not sure.
  • Include the changelog in a file named: doc/changes/<PR number>.<type> 'type' can be one of the following: feature, bugfix, doc, removal, misc, or deprecation (see here for more information).

Do the same change for plot_wigner_sphere and matrix_histogram as #2170 did.
#2170

  1. The arguments in plot_wigner_sphere is now similar to those of other functions. Of course, it change its color as users set qutip.settings.colorblind_safe=True.
def plot_wigner_sphere(wigner, reflections=False, *, cmap=None,
                       colorbar=True, fig=None, ax=None):
  1. matrix_histogram and matrix_histogram_complex combined into one function. The new matrix_histogram can change bar_style and color_style. Here is an example.
matrix_histogram

@coveralls
Copy link

coveralls commented Jul 10, 2023

Coverage Status

coverage: 84.621% (+1.4%) from 83.199% when pulling 22f94fa on tamakoshi2001:edit_functions_in_visualization.py into 4f29ae5 on qutip:master.

@tamakoshi2001 tamakoshi2001 changed the title make plot_wigner_sphere user-friendly change for plot_wigner_sphere and matrix_histogram_complex Jul 10, 2023
@tamakoshi2001
Copy link
Contributor Author

Codeclimte detects the same code in matrix_histogram and matrix_histogram_complex, but matrix_histogram_complex will be deleted after matrix_histogram takes over it and the problem will be solved

@tamakoshi2001
Copy link
Contributor Author

sample plot of matrix_histogram
matrix-histogram.pdf

@tamakoshi2001
Copy link
Contributor Author

I moved limits bar_style color_limits color_style. The number of arguments is 13 > 10, but they should not be in options. Also, I raised an error when users pass unexpected arguments to bar_style color_style.

@tamakoshi2001
Copy link
Contributor Author

tamakoshi2001 commented Jul 12, 2023

@Ericgig Hi. Do you have any advice on plot_wigner_sphere and matrix_histogram? I will fix the documentation error tomorrow.

@tamakoshi2001
Copy link
Contributor Author

@Ericgig Hi. I tried to fix the documentation error, but I do not know how to fix the following errors. Could you help me?

/home/runner/work/qutip/qutip/qutip/visualization.py:docstring of qutip.visualization.hinton:10: WARNING: Bullet list ends without a blank line; unexpected unindent.
looking for now-outdated files... none found
/home/runner/work/qutip/qutip/qutip/visualization.py:docstring of qutip.visualization.hinton:7: WARNING: Bullet list ends without a blank line; unexpected unindent.
/home/runner/work/qutip/qutip/qutip/visualization.py:docstring of qutip.visualization.hinton:23: WARNING: Bullet list ends without a blank line; unexpected unindent.

Comment on lines 274 to 282
- If set to ``"scaled"`` (default), each color is chosen by
passing the absolute value of the corresponding matrix
element into `cmap` with the sign of the real part.
passing the absolute value of the corresponding matrix
element into `cmap` with the sign of the real part.
- If set to ``"threshold"``, each square is plotted as
the maximum of `cmap` for the positive real part and as
the minimum for the negative part of the matrix element;
note that this generalizes `"threshold"` to complex numbers.
the maximum of `cmap` for the positive real part and as
the minimum for the negative part of the matrix element;
note that this generalizes `"threshold"` to complex numbers.
- If set to ``"phase"``, each color is chosen according to
the angle of the corresponding matrix element.
the angle of the corresponding matrix element.
Copy link
Member

Choose a reason for hiding this comment

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

I think it's these bullet list that cause the issues with the documentation.
The indentation is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! This solved the problem. The other two are in matrix_histogram.

@Ericgig Ericgig mentioned this pull request Jul 14, 2023
5 tasks
@tamakoshi2001
Copy link
Contributor Author

@Ericgig This PR is. also ready for review. The build doc fails, but it is not related to my change.

Copy link
Member

@Ericgig Ericgig left a comment

Choose a reason for hiding this comment

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

A few issues to fix, but overall looks nice.

qutip/visualization.py Outdated Show resolved Hide resolved
@@ -679,12 +720,10 @@ def matrix_histogram(M, xlabels=None, ylabels=None, title=None, limits=None,
"""

# default options
default_opts = {'figsize': None, 'cmap': 'jet', 'cmap_min': 0.,
'cmap_max': 1., 'zticks': None, 'bars_spacing': 0.2,
default_opts = {'zticks': None, 'bars_spacing': 0.2,
Copy link
Member

Choose a reason for hiding this comment

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

Could you rename this variable, it is used later with values no longer the default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the code to use options argument instead of default_opts and changed the default value from None to {}.

qutip/visualization.py Outdated Show resolved Hide resolved
@tamakoshi2001
Copy link
Contributor Author

@Ericgig Hi. I fixed the issues you suggested.


'threshold': float, optional
Threshold for when bars of smaller height should be transparent. If
not set, all bars are colored according to the color map.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
not set, all bars are colored according to the color map.
not set, all bars are colored according to the color map.

Copy link
Member

@Ericgig Ericgig left a comment

Choose a reason for hiding this comment

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

One last small change.

@tamakoshi2001
Copy link
Contributor Author

@Ericgig I added an indent. The doc looks good now.

@Ericgig Ericgig merged commit efd39be into qutip:master Jul 19, 2023
11 of 12 checks passed
@tamakoshi2001 tamakoshi2001 changed the title change for plot_wigner_sphere and matrix_histogram_complex Update plot_wigner_sphere and delete matrix_histogram_complex Aug 28, 2023
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.

None yet

3 participants