Skip to content

Draft: Contour kwarg for negative_linestyles #23102

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

Closed
wants to merge 145 commits into from

Conversation

andrew-fennell
Copy link
Contributor

@andrew-fennell andrew-fennell commented May 22, 2022

PR Summary

I made the negative_linestyles for contours toggle-able via kwarg. The only way to toggle this currently is with rcParams.

In the attached issue, we had some discussion on whether this is a good idea or not, but I think it is worth considering. We can maintain current functionality and just apply the kwarg if it is defined. This will not break previous functionality. I will make tests and write documentation/examples as well.

Please use this PR and the attached issue to discuss whether this is a good idea or not. I'm happy to take this in a different direction if there are some better ideas!

Closes #23028

Previous

plt.rcParams['contour.negative_linestyle'] = 'solid'
fig, ax = plt.subplots()
CS = ax.contour(X, Y, Z, 6, colors='k')  # Negative contours default to dashed.
ax.clabel(CS, fontsize=9, inline=True)
ax.set_title('Single color - negative contours solid')

With this PR

fig2, ax2 = plt.subplots()
CS = ax2.contour(X, Y, Z, 6, colors='k', negative_linestyles='solid')
ax2.clabel(CS, fontsize=9, inline=True)
ax2.set_title('Single color - negative contours solid')

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).

@andrew-fennell andrew-fennell marked this pull request as draft May 22, 2022 22:15
@@ -1764,6 +1769,17 @@ def _initialize_x_y(self, z):
iterable is shorter than the number of contour levels
it will be repeated as necessary.

negative_linestyles : {*None*, 'solid', 'dashed', 'dashdot', 'dotted'}, optional
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the proper way to break this line to meet line length requirements?

Copy link
Member

Choose a reason for hiding this comment

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

*None* or string, optional and then list the acceptable strings below?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not convinced it should be *None*, as *...* is used to mark parameter names, not values. Although it is clearly used in contour.py and in a few other locations.

Grepping a bit, a plain None seems to be the most common option. (Although *None* is used in > 100 locations in the code.)

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 did the same thing and found mixed results. I am happy to remove ... if that is the consensus.

I also found a mix of "None or str", and "str or None"...

Copy link
Member

Choose a reason for hiding this comment

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

I'm hoping that someone will come in with a consensus statement. :-)

But I'm pretty sure that not using *None* will be less controversial than using it. Also, I believe that in the text it should either be a plain None or a double single-backtick one (to render as code), although the former is clearly the most common.

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 in rst it's None, but not sure about the docstrings.

negative_linestyles=None, hatches=(None,), alpha=None,
origin=None, extent=None, cmap=None, colors=None, norm=None,
vmin=None, vmax=None, extend='neither', antialiased=None,
nchunk=0, locator=None, transform=None,
Copy link
Member

Choose a reason for hiding this comment

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

I'd position the new kw-arg last.

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 for letting me know!

neg_ls = mpl.rcParams['contour.negative_linestyle']
# If negative_linestyles was not defined as a kwarg,
# define negative_linestyles with rcParams
if not neg_ls:
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
if not neg_ls:
if neg_ls is None:

negative_linestyle='' would otherwise lead to surprising results.

Should you also set self.negative_linestyles here? I think it can make sense to not use the neg_ls local variable and just use self.negative_linestyles throughout the code here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In that case, I'll move the case with rcParams['contour.negative_linestyle'] to where self.negative_linestyles is initially defined.

tacaswell and others added 23 commits June 13, 2022 23:14
Bumps [docker/setup-qemu-action](https://github.com/docker/setup-qemu-action) from 1 to 2.
- [Release notes](https://github.com/docker/setup-qemu-action/releases)
- [Commits](docker/setup-qemu-action@v1...v2)

---
updated-dependencies:
- dependency-name: docker/setup-qemu-action
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
read_text/write_text default to the locale encoding, which may or may
not be utf8.  Fix these by making the encoding explicit or by using
bytes.
I don't think other encodings really worked previously, so there's no
deprecation period.
For `FontProperties(stretch=12345)`; before:
```
Traceback (most recent call last):
  File ".../matplotlib/font_manager.py", line 888, in set_stretch
    raise ValueError()
ValueError

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File ".../matplotlib/font_manager.py", line 695, in __init__
    self.set_stretch(stretch)
  File ".../matplotlib/font_manager.py", line 891, in set_stretch
    raise ValueError("stretch is invalid") from err
ValueError: stretch is invalid
```
after
```
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File ".../matplotlib/font_manager.py", line 695, in __init__
    self.set_stretch(stretch)
  File ".../matplotlib/font_manager.py", line 900, in set_stretch
    raise ValueError(f"{stretch=} is invalid")
ValueError: stretch=12345 is invalid
```
For tex failures (e.g., `figtext(.5, .5, "\N{snowman}", usetex=True)`),
instead of
```
Traceback (most recent call last):
  File ".../matplotlib/texmanager.py", line 253, in _run_checked_subprocess
    report = subprocess.check_output(
  File "/usr/lib/python3.10/subprocess.py", line 420, in check_output
    return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
  File "/usr/lib/python3.10/subprocess.py", line 524, in run
    raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '['latex', '-interaction=nonstopmode', '--halt-on-error', '../71cab2b5aca12ed5cad4a481b3b00e42.tex']' returned non-zero exit status 1.

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
    <a long traceback>
    raise RuntimeError(
RuntimeError: latex was not able to process the following string:
b'\\u2603'

Here is the full report generated by latex:
This is pdfTeX, Version 3.141592653-2.6-1.40.23 (TeX Live 2021) (preloaded format=latex)
<the tex output>
```

instead report the failure in a single block, as
```
Traceback (most recent call last):
    <a long traceback>
    raise RuntimeError(
RuntimeError: latex was not able to process the following string:
b'\\u2603'

Here is the full command invocation and its output:

latex -interaction=nonstopmode --halt-on-error ../71cab2b5aca12ed5cad4a481b3b00e42.tex

This is pdfTeX, Version 3.141592653-2.6-1.40.23 (TeX Live 2021) (preloaded format=latex)
<the tex output>
```
(the exception chaining is not particularly informative, and we can move
the only relevant info -- the command we tried to run -- to the second
exception).
This fixes the following error in mingw gcc toolchain. Also clang
also have same error.

src/_tkagg.cpp:274:26: note:   crosses initialization of 'bool tk_ok'
  274 |     bool tcl_ok = false, tk_ok = false;
      |                          ^~~~~
src/_tkagg.cpp:274:10: note:   crosses initialization of 'bool tcl_ok'
  274 |     bool tcl_ok = false, tk_ok = false;
      |          ^~~~~~

According to C++ standard (6.7.3):
It is possible to transfer into a block, but not in a way that bypasses
declarations with initialization. A program that jumps from a point where
a variable with automatic storage duration is not in scope to a point
where it is in scope is ill-formed unless the variable has scalar type...
This adds the rcParams savefig.directory option into the macosx
backend for the savefig dialog window.
fill_between isn't the right thing to use here. It might do the same
thing (as long as the plot isn't interacted with / zoomed out), but it
feels real clunky and should not be encouraged to use here in a user
facing example. I only encountered this because this plot was showing up
as a usage example on the plt.fill_between() API docs (older version)
and I found this real odd.
Arrays sometimes don't have all the methods arrays should have, so
add another check here.  Plot requires both ndim and shape and this
will extract the numpy array if x does not have those attributes.
Otherwise leave the object alone, because unit support (currently only
in plot) requires the object to retain the unit info.
melissawm and others added 24 commits June 13, 2022 23:14
When an overunder symbol has a subscript, it goes below the symbol, and the baseline of the Vlist thus created is actually the baseline of the lowest item in the Vlist. Hence, it must be moved only if there is a subscript, and not otherwise.
Adds compress=True to compressed layout engine.  Works for compact
axes grids.
Closes matplotlib#23114, where somebody has installed matplotlib into another
git repo.
The *math* parameter is passed through many layers of the call stack
but is ultimately only used for a single purpose: deciding whether to
replace the ASCII hyphen by a (longer) unicode minus.  Instead of doing
that, just do the substitution at the parsing stage.  In particular,
this fixes problematic unicode minus support with the "cm" fontset.

This patch also reverts a significant part of 52003e4, as LogFormatters
no longer need to pass unicode minuses in mathtext -- everything gets
converted by mathtext.  Likewise, this change also invalidates the
test_log_scales baseline image (old, buggy wrt. unicode minus); replace
it by a test that the drawn ticks are as expected (which was the intent
in 90c1aa3).
AFAICT, (incorrect) support for the double-backslashed version was
accidentally introduced in 027dd2c.
This also ensures consistency in the names in the error strings; see
e.g. changes in test_mathtext (the contents of `\overline` are indeed a
"body", not a "value").

Renaming "simple_group" to "optional_group" is semantically more
meaningful.
@andrew-fennell
Copy link
Contributor Author

Moving to new branch: #23266

@andrew-fennell andrew-fennell deleted the contour-linestyle branch June 14, 2022 04:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ENH]: contour kwarg for negative_linestyle