Repository: matplotlib/matplotlib
Pull request: Force origin='upper' in pyplot.specgram
## PR Summary
Fix #17878

In ``pyplot.specgram``, the ``origin`` kwarg is popped. The ``origin`` is explicitly set to 'upper' 
in the call to ``imshow`` inside ``pyplot.specgram``. Test added to check that changing the 
``image.origin`` rcParam and the ``origin`` kwarg to 'lower' does not change the spectrogram. 
Documentation updated with note that ``image.origin`` rcParam and ``origin`` kwarg are ignored.

## PR Checklist

- [x] Has Pytest style unit tests
- [x] Code is [Flake 8](http://flake8.pycqa.org/en/latest/) compliant
- [ ] New features are documented, with examples if plot related
- [x] Documentation is sphinx and numpydoc compliant
- [ ] Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
- [x] Documented in doc/api/next_api_changes/* if API changed in a backward-incompatible way

<!--
Thank you so much for your PR!  To help us review your contribution, please
consider the following points:

- A development guide is available at https://matplotlib.org/devdocs/devel/index.html.

- Help with git and github is available at
  https://matplotlib.org/devel/gitwash/development_workflow.html.

- Do not create the PR out of master, but out of a separate branch.

- The PR title should summarize the changes, for example "Raise ValueError on
  non-numeric input to set_xlim".  Avoid non-descriptive titles such as
  "Addresses issue #8576".

- The summary should provide at least 1-2 sentences describing the pull request
  in detail (Why is this change required?  What problem does it solve?) and
  link to any relevant issues.

- If you are contributing fixes to docstrings, please pay attention to
  http://matplotlib.org/devel/documenting_mpl.html#formatting.  In particular,
  note the difference between using single backquotes, double backquotes, and
  asterisks in the markup.

We understand that PRs can sometimes be overwhelming, especially as the
reviews start coming in.  Please let us know if the reviews are unclear or
the recommended next step seems overly demanding, if you would like help in
addressing a reviewer's comments, or if you have been waiting too long to hear
back on your PR.
-->


Problem statement:
flipping of imshow in specgram
<!--To help us understand and resolve your issue, please fill out the form to the best of your ability.-->
<!--You can feel free to delete the sections that do not apply.-->

### Bug report

**Bug summary**

The function 
```python
matplotlib.pyplot.specgram
```
uses imshow internally to plot a spectrogram but does not set the origin. So someone can externally overwrite the standard behaviour and wonder, why the spectrogramm is flipped.

Unintensionally, this can even happen if another imshow was used before with different origin.

<!--A short 1-2 sentences that succinctly describes the bug-->

**Code for reproduction**

For reproduction, only the origin setting before the function call is added to the standard demo and flips the output.

<!--A minimum code snippet required to reproduce the bug.
Please make sure to minimize the number of dependencies required, and provide
any necessary plotted data.
Avoid using threads, as Matplotlib is (explicitly) not thread-safe.-->

```python
import matplotlib.pyplot as plt
import numpy as np
np.random.seed(19680801)
dt = 0.0005
t = np.arange(0.0, 20.0, dt)
s1 = np.sin(2 * np.pi * 100 * t)
s2 = 2 * np.sin(2 * np.pi * 400 * t)
s2[t <= 10] = s2[12 <= t] = 0
nse = 0.01 * np.random.random(size=len(t))
x = s1 + s2 + nse  # the signal
NFFT = 1024  # the length of the windowing segments
Fs = int(1.0 / dt)  # the sampling frequency
fig, (ax1, ax2) = plt.subplots(nrows=2)
ax1.plot(t, x)

# now overwrite the setting to get the spectrogramm flipped
plt.rcParams["image.origin"] = 'lower'

Pxx, freqs, bins, im = ax2.specgram(x, NFFT=NFFT, Fs=Fs, noverlap=900)
plt.show()
```

**Actual outcome**

Flipping of the spectrogram without flipping the y-axis.

**Expected outcome**

Non-flipping independent of origin setting of imshow or specifically, "image.origin" of rcParams.

<!--A description of the expected outcome from the code snippet-->
<!--If this used to work in an earlier version of Matplotlib, please note the version it used to work on-->

**Matplotlib version**

3.2.1
conda installation
<!--Please tell us how you installed matplotlib and python e.g., from source, pip, conda-->
<!--If you installed from conda, please specify which channel you used if not the default-->

**Suggestion**

Someone with the knowledge should add the keyword origin='upper' to the relevant imshow command of specgram.

Hints from issue discussion:
The semantics around extent / origin / the default extents is a bit complicated (see https://matplotlib.org/tutorials/intermediate/imshow_extent.html). 

The relevant code for this is at https://github.com/matplotlib/matplotlib/blob/ec0132f66170dabc6352ba79d839b1dd9eb02157/lib/matplotlib/axes/_axes.py#L7551-L7552 I think it makes sense that if we are pinning the extent we should also pin the origin.
The work here is to:

 - make sure that setting the origin the inner call to imshow in `specgram` is actually the correct fix
 - add a test (via check_figures_equal)
 - document a behavior change
I would like to work on this for the SciPy 2020 sprint.
I am also looking into this as part of the SciPy 2020 sprint.
sorry for not being clear.  What I mean by "pin" is "ensure that in `spcegram` we always pass `origin='upper'` to the underlying `imshow` call independent of what the rcparam is"


@thedorathanasiadis and I had a brief verbal discussion about this too (that I should have moved to this issue in text, sorry). that  we have two ways to fix this:

 1. make `origin='upper'` work correctly for `specgram` and sort out how to change the ylimits etc and add `origin` as a kwarg to `specgram`.  This we be adding a new feature that, given the context of `specgram` 
 2. make `specgram` ignore the rcparam and prevent the user from passing in `origin` as a kwarg

I am inclined to option 2 (it is simpler and given that I think this has been broken forever no one is clamoring for this feature).  Going with option 2 now does not prevent us from being able to do option 1 later, but if we go with option 1 now we can't go back.

Commit message: DOC: Remove note that origin='upper' in specgram

This note referenced the call to imshow inside of specgram, which might confuse users who do not know the implementation of specgram.

Patch to review:
diff --git a/doc/api/next_api_changes/behavior/17897-KLP.rst b/doc/api/next_api_changes/behavior/17897-KLP.rst
new file mode 100644
index 000000000000..9b8ee78c7e8a
--- /dev/null
+++ b/doc/api/next_api_changes/behavior/17897-KLP.rst
@@ -0,0 +1,11 @@
+pyplot.specgram uses origin='upper' even if an different kwarg or rcParam value is set  
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+Previously if ``image.origin`` was set to something other than upper or if 
+the ``origin`` kwarg was passed with a value other than upper, the spectrogram
+itself would flip, but the axes would remain oriented for an origin value 
+of 'upper', so that the resulting plot was incorrectly labelled.
+
+Now, the ``origin`` kwarg is not supported and the ``image.origin`` rcParam is ignored.
+The function matplotlib.pyplot.specgram is forced to use ``origin='upper'``, so that
+the axes are correct for the plotted spectrogram.
diff --git a/lib/matplotlib/axes/_axes.py b/lib/matplotlib/axes/_axes.py
index 40cd2aefa911..9a9e5f84e0d5 100644
--- a/lib/matplotlib/axes/_axes.py
+++ b/lib/matplotlib/axes/_axes.py
@@ -7464,8 +7464,8 @@ def specgram(self, x, NFFT=None, Fs=None, Fc=None, detrend=None,
 
         **kwargs
             Additional keyword arguments are passed on to `~.axes.Axes.imshow`
-            which makes the specgram image.
-
+            which makes the specgram image. The origin keyword argument
+            is not supported.
         Returns
         -------
         spectrum : 2-D array
@@ -7548,8 +7548,13 @@ def specgram(self, x, NFFT=None, Fs=None, Fc=None, detrend=None,
         xmin, xmax = xextent
         freqs += Fc
         extent = xmin, xmax, freqs[0], freqs[-1]
+
+        if 'origin' in kwargs:
+            raise TypeError("specgram() got an unexpected keyword argument "
+                            "'origin'")
+
         im = self.imshow(Z, cmap, extent=extent, vmin=vmin, vmax=vmax,
-                         **kwargs)
+                         origin='upper', **kwargs)
         self.axis('auto')
 
         return spec, freqs, t, im
diff --git a/lib/matplotlib/tests/test_axes.py b/lib/matplotlib/tests/test_axes.py
index cbc6a5cdd4a5..a7a4b2561a5b 100644
--- a/lib/matplotlib/tests/test_axes.py
+++ b/lib/matplotlib/tests/test_axes.py
@@ -4099,6 +4099,33 @@ def test_specgram_fs_none():
     assert xmin == 32 and xmax == 96
 
 
+@check_figures_equal(extensions=["png"])
+def test_specgram_origin_rcparam(fig_test, fig_ref):
+    """Test specgram ignores image.origin rcParam and uses origin 'upper'."""
+    t = np.arange(500)
+    signal = np.sin(t)
+
+    plt.rcParams["image.origin"] = 'upper'
+
+    # Reference: First graph using default origin in imshow (upper),
+    fig_ref.subplots().specgram(signal)
+
+    # Try to overwrite the setting trying to flip the specgram
+    plt.rcParams["image.origin"] = 'lower'
+
+    # Test: origin='lower' should be ignored
+    fig_test.subplots().specgram(signal)
+
+
+@pytest.mark.xfail(strict=True)
+def test_specgram_origin_kwarg():
+    """Ensure passing origin as a kwarg raises an exception."""
+    t = np.arange(500)
+    signal = np.sin(t)
+
+    plt.specgram(signal, origin='lower')
+
+
 @image_comparison(
     ["psd_freqs.png", "csd_freqs.png", "psd_noise.png", "csd_noise.png"],
     remove_text=True, tol=0.002)
diff --git a/tutorials/introductory/customizing.py b/tutorials/introductory/customizing.py
index 97baedda6427..ab761fb8bcaf 100644
--- a/tutorials/introductory/customizing.py
+++ b/tutorials/introductory/customizing.py
@@ -12,7 +12,8 @@
 <customizing-with-matplotlibrc-files>` file (which is read at startup to
 configure Matplotlib).
 
-There are a number of pre-defined styles `provided by Matplotlib`_. For
+There are a number of pre-defined styles :doc:`provided by Matplotlib
+</gallery/style_sheets/style_sheets_reference>`. For
 example, there's a pre-defined style called "ggplot", which emulates the
 aesthetics of ggplot_ (a popular plotting package for R_). To use this style,
 just add:
@@ -202,4 +203,3 @@
 #
 # .. _ggplot: https://ggplot2.tidyverse.org/
 # .. _R: https://www.r-project.org/
-# .. _provided by Matplotlib: https://github.com/matplotlib/matplotlib/tree/master/lib/matplotlib/mpl-data/stylelib


Code review comments:
diff_hunk: @@ -4099,6 +4099,33 @@ def test_specgram_fs_none():
     assert xmin == 32 and xmax == 96
 
 
+@check_figures_equal(extensions=["png"])
+def test_specgram_origin_rcparam(fig_test, fig_ref):
+    """Test specgram ignores image.origin rcParam and uses origin 'upper'."""
+    t = np.arange(500)
+    signal = np.sin(t)
+
+    plt.rcParams["image.origin"] = 'upper'
+
+    # Reference: First graph using default origin in imshow (upper),
+    fig_ref.subplots().specgram(signal)
+
+    # Try to overwrite the setting trying to flip the specgram
+    plt.rcParams["image.origin"] = 'lower'
+
+    # Test: origin='lower' should be ignored
+    fig_test.subplots().specgram(signal)
+
+
+@pytest.mark.xfail(strict=True)
path: lib/matplotlib/tests/test_axes.py
text: @user1:
This is not an expected failure; use `pytest.raises` to check the exception is raised.

@author:
Thank you so much for all of the suggestions.