Allow both linestyle definition "accents" and dash-patterns as linestyle #3772

lennart0901 · 2014-11-09T13:59:14Z

This is a first PR in an attempt to split #3265 in smaller pieces.
This part should cover the original intent to fix #2136.

The reverse mapper is necessary here to convert full line style names in short ones for lines.py.

tacaswell · 2014-11-10T11:14:32Z

lib/matplotlib/lines.py

@@ -957,8 +972,8 @@ def set_linestyle(self, linestyle):
                break

        if linestyle not in self._lineStyles:
-            if linestyle in ls_mapper:
-                linestyle = ls_mapper[linestyle]
+            if linestyle in ls_mapper_r:


Can you use the same ls_mapper_r.get(ls, ls) pattern used above?

tacaswell · 2014-11-10T11:30:16Z

modulo some small comments, this looks good to me.

tacaswell · 2014-11-11T02:51:49Z

lib/matplotlib/cbook.py

@@ -2108,7 +2108,10 @@ def unmasked_index_ranges(mask, compressed=True):
               (':', 'dotted')]

 ls_mapper = dict(_linestyles)
-ls_mapper.update([(ls[1], ls[0]) for ls in _linestyles])


This may be neurotic, but should this be documented in api_changes.rst?

You are the maintainer. :)

Is there some order in the file?

It should be more-or-less by date. Can you actually throw it into this folder (https://github.com/matplotlib/matplotlib/tree/master/doc/api/api_changes)? This is an attempt to alleviate the merge conflicts in the docs.

Argh, to late I just added a note at the end of the Changes 1.4.x

It definitely should not go there as I don't think this is going to be back-ported to the 1.4 series.

Fixed that.

jenshnielsen · 2014-11-12T09:52:06Z

This is failing due to the issue fixed by #3784

tacaswell · 2014-11-12T13:45:13Z

@mdboom @efiring @pelson @WeatherGod Can one of you have a look at this? It looks good to me, but given that this touches things rather deep in the library I would like a second opinion.

@lennart0901 One more documentation thing, can you also add an entry in the whats_new folder? The purpose of that is to advertise in the release notes the changes and I think unifying the linestyle inputs is worth advertising!

lennart0901 · 2014-11-12T15:30:13Z

Well there is still one little thing left. Patches and Collections do not accept "None" and the like (no line drawn). Shall I try to fix that as well? I'm still not sure if that should be done on backend level or for the artists.

tacaswell · 2014-11-12T16:19:38Z

I think that should be fixed in a different PR. I am always going to favor more small PRs over fewer bigger ones.

If I understand everything right, as-is this is a major improvement and (almost) unifies the api and nothing is broken due to this omission.

I am not sure if no-line makes sense for patches/collections (I think it turns into zero-width edges?)

lennart0901 · 2014-11-12T16:33:36Z

I am not sure if no-line makes sense for patches/collections (I think it turns into zero-width edges?)

Exactly, it would be an alternative to zero width.

tacaswell · 2014-11-13T00:14:35Z

Failure is a fluke, restarted 2.6 test.

WeatherGod · 2014-11-17T16:31:43Z

ping @tacaswell, the tests pass now.

tacaswell · 2014-11-18T01:59:38Z

lib/matplotlib/lines.py

-            else:
-                verbose.report('Unrecognized line style %s, %s' %
-                               (linestyle, type(linestyle)))
+            linestyle = ls_mapper_r.get(linestyle, linestyle)


Shouldn't this still raise an exception if linestyle is not in either set?

In [2]: ln, = plt.plot(range(5)) In [8]: ln.set_linestyle('aardvark') In [9]: plt.draw()

does not raise an exception, but I think it should.

I have a feeling I suggested you make this change and have now changed my mind about it, sorry.

I have a PR in against your branch to fix this.

and seems to break a whole lot of other things....

You are referring to your PR, aren't you

@lennart0901 The test failure should be fixed up now (fingers crossed on travis).

lennart0901 · 2014-11-21T19:15:52Z

@tacaswell: I merged your PR.

tacaswell · 2014-11-27T04:07:52Z

lib/matplotlib/collections.py

@@ -463,16 +463,33 @@ def set_linestyle(self, ls):
        """
        Set the linestyle(s) for the collection.

-        ACCEPTS: ['solid' | 'dashed', 'dashdot', 'dotted' |
-        (offset, on-off-dash-seq) ]
+        ===========================   =================


@mdboom @efiring This breaks the doc-string scraping code for the tables in the docs. Should we revert the changes to put ACCEPTS : ... back in or re-work the doc-scraping?

I added a PR to demonstrate docscraping using numpydoc: #3859

tacaswell · 2014-11-28T17:16:21Z

My concern with using numpydoc for this is than now it is a run-time dependency, not just a documentation-build dependency.

lennart0901 · 2014-11-28T18:12:53Z

I answered on #3859

tacaswell · 2015-04-11T23:04:02Z

@lennart0901 This needs a re-base.

…yle for collection, lines and patches. (code & tests)

lennart0901 · 2015-04-12T11:22:02Z

I updated the baseline image of the test that failed. Somehow text spacings changed slightly.

WeatherGod · 2015-04-13T15:50:01Z

lib/matplotlib/tests/test_collections.py

@@ -325,6 +325,18 @@ def test__EventCollection__set_linestyle():
    splt.set_title('EventCollection: set_linestyle')


+@image_comparison(baseline_images=['EventCollection_plot__set_ls_dash'])


add the "remove_text=True" kwarg and update the test image if you aren't going to test text. This helps deal with the tiny differences that occur with different versions of freetype.

In this case we also had a known 'remake all the tests' change to text layout PR in between.

@tacaswell Thanks for the reminder. I havn't touched mpl code for a while now. I'll check the Changelog more regulary.

The test is corrected now.

tacaswell · 2015-04-13T19:04:36Z

Can you also do some re-basing to remove the images we are not using from the history? We are trying to keep the repository size down (or to keep it from getting any bigger than it currently is).

… seperate image (to allow multiprocess testing...) and remove text

…nes.

…s_mapper

…getting getting value from ls_mapper_r

…ection

Added check in code + test for raise

lennart0901 · 2015-04-14T06:36:37Z

Done. Also squashed moving the api_changes from api_changes.rst to the api_changes folder into the commit where the documentation is first created.

pelson · 2015-04-21T06:46:48Z

lib/matplotlib/tests/test_collections.py

@@ -325,6 +325,19 @@ def test__EventCollection__set_linestyle():
    splt.set_title('EventCollection: set_linestyle')


+@image_comparison(baseline_images=['EventCollection_plot__set_ls_dash'],


Just a question: does this need to test all of the backends, or can it target specific ones? e.g., is a png test sufficient?

It would be ok to test just one backend. I only want to test that the linestyle is set and interpreted correctly.
But I think that is true for a number of tests in this file...

efiring · 2015-05-13T18:30:53Z

It looks like this is held up by the doc formatting change, correct? Maybe it makes sense to revert that part of the change and then merge this?

API/ENH : Allow both linestyle definition "accents" and dash-patterns as linestyle

tacaswell · 2015-05-14T03:57:53Z

I concur, this has been sitting far too long.

DOC : revert some documentation changes from #3772

tacaswell · 2015-05-14T12:29:53Z

@lennart0901 Thank you for this! Sorry it too so long to get it merged, the hold up was entirely on our end.

lennart0901 · 2015-05-14T16:13:29Z

All fine and thanks for preparing the reversion of the docstrings. I'm too busy at the moment.

tacaswell added this to the v1.5.x milestone Nov 9, 2014

tacaswell added the status: needs review label Nov 9, 2014

tacaswell reviewed Nov 10, 2014
View reviewed changes

lennart0901 force-pushed the linestyle_#2136_2 branch from 1bb7fab to f46a56a Compare November 10, 2014 16:11

tacaswell reviewed Nov 11, 2014
View reviewed changes

tacaswell added status: needs revision and removed status: needs review labels Nov 12, 2014

lennart0901 force-pushed the linestyle_#2136_2 branch from 73db6d3 to 50613a0 Compare November 12, 2014 05:44

tacaswell added status: needs review and removed status: needs revision labels Nov 12, 2014

tacaswell reviewed Nov 18, 2014
View reviewed changes

tacaswell added the status: needs revision label Nov 18, 2014

lennart0901 mentioned this pull request Nov 18, 2014

Allow both linestyle definition "accents" and dash-patterns as linestyle... #3265

Closed

tacaswell reviewed Nov 27, 2014
View reviewed changes

tacaswell removed the status: needs revision label Nov 27, 2014

lennart0901 force-pushed the linestyle_#2136_2 branch from 859dac2 to 8d00efb Compare November 28, 2014 15:30

Allow both linestyle definition "accents" and dash-patterns as linest…

31b7b91

…yle for collection, lines and patches. (code & tests)

lennart0901 force-pushed the linestyle_#2136_2 branch from 8d00efb to 482c909 Compare April 12, 2015 10:54

WeatherGod reviewed Apr 13, 2015
View reviewed changes

lennart0901 force-pushed the linestyle_#2136_2 branch from 14345ca to b8ced84 Compare April 13, 2015 18:27

lennart0901 and others added 11 commits April 14, 2015 08:32

Make baseline image for testing single_dash pattern in collections.py…

14fe5db

… seperate image (to allow multiprocess testing...) and remove text

Add new documentation for set_linestyle of patches, collection and li…

8d1f0e0

…nes.

Rename ls_mapperr to ls_mapper_r and add comment about the split of l…

a18bad9

…s_mapper

Rename a test_linestyle_accent to test_linestyle_variants and change …

3fc7a0b

…getting getting value from ls_mapper_r

Add test for specifying a single dash pattern as linestyle for a coll…

9108a4b

…ection

Document split of matplotlib.cbook.ls_mapper in api_changes

0ec4e67

Add new entry for unified linestyle handling to whats_new.

e6718ca

ENH : raise on invalid linestyle

a09d4ad

Added check in code + test for raise

BUG : properly handle 'none' as linestyle

8b90be7

TST : import nose so 2.6 fails gracefully

7858040

Correct parameter name to match documentation in Line2D.set_linestyle

937248d

lennart0901 force-pushed the linestyle_#2136_2 branch from b8ced84 to 937248d Compare April 14, 2015 06:34

pelson reviewed Apr 21, 2015
View reviewed changes

tacaswell added a commit that referenced this pull request May 14, 2015

Merge pull request #3772 from lennart0901/linestyle_#2136_2

6376047

API/ENH : Allow both linestyle definition "accents" and dash-patterns as linestyle

tacaswell merged commit 6376047 into matplotlib:master May 14, 2015

tacaswell removed the status: needs review label May 14, 2015

tacaswell added a commit to tacaswell/matplotlib that referenced this pull request May 14, 2015

DOC : revert some documentation changes from matplotlib#3772

5f39071

efiring added a commit that referenced this pull request May 14, 2015

Merge pull request #4427 from tacaswell/doc_linestyle_accepts

bbe291b

DOC : revert some documentation changes from #3772

		@@ -325,6 +325,18 @@ def test__EventCollection__set_linestyle():
		splt.set_title('EventCollection: set_linestyle')


		@image_comparison(baseline_images=['EventCollection_plot__set_ls_dash'])

		@@ -325,6 +325,19 @@ def test__EventCollection__set_linestyle():
		splt.set_title('EventCollection: set_linestyle')


		@image_comparison(baseline_images=['EventCollection_plot__set_ls_dash'],

Allow both linestyle definition "accents" and dash-patterns as linestyle #3772

Allow both linestyle definition "accents" and dash-patterns as linestyle #3772

Conversation

lennart0901 commented Nov 9, 2014

Choose a reason for hiding this comment

tacaswell commented Nov 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenshnielsen commented Nov 12, 2014

tacaswell commented Nov 12, 2014

lennart0901 commented Nov 12, 2014

tacaswell commented Nov 12, 2014

lennart0901 commented Nov 12, 2014

tacaswell commented Nov 13, 2014

WeatherGod commented Nov 17, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lennart0901 commented Nov 21, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell commented Nov 28, 2014

lennart0901 commented Nov 28, 2014

tacaswell commented Apr 11, 2015

lennart0901 commented Apr 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell commented Apr 13, 2015

lennart0901 commented Apr 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efiring commented May 13, 2015

tacaswell commented May 14, 2015

tacaswell commented May 14, 2015

lennart0901 commented May 14, 2015