support for updating axis ticks for categorical data #6889

Merged
merged 1 commit into from Aug 25, 2016

Conversation

Projects
None yet
3 participants
Member

story645 commented Aug 3, 2016

Now supports updating axis:

fig, ax = plt.subplots()
ax.plot(['a', 'c', 'e'], label="plot 1")
ax.plot(['a', 'b', 'd'], label="plot 2")
ax.plot(['b', 'e', 'd'], label="plot3")
ax.legend()

unknown

Which as a sidenote, it didn't work before because apparently FixedLocator converts to an array as soon as it's passed in (and also apparently it doesn't need to...)

UnitData was also changed into an object to facilitate the updating and as a precursor to adding more functionality. And the tests should now be py.test compliant @Kojoley

@story645 story645 commented on an outdated diff Aug 3, 2016

lib/matplotlib/category.py
+ def __init__(self, data):
+ """Create mapping between unique categorical values
+ and numerical identifier
+ Paramters
+ ---------
+ data: iterable
+ sequence of values
+ """
+ self.seq, self.locs = [], []
+ self._set_seq(data)
+ self._set_locs(0)
+
+ def update(self, new_data):
+ self._set_seq(new_data)
+ value = max(self.locs)
+ self._set_locs(value + 1)
@story645

story645 Aug 3, 2016

Member

needs to be max(value +1, 0) to not conflict with spdict conventions

Member

story645 commented Aug 3, 2016 edited

I may have to spin off a PR on supporting both pytest and nosetest on appveyor/travis if I ever want my tests to pass again - help @Kojoley?

Member

Kojoley commented Aug 3, 2016

I may have to spin off a PR on supporting both pytest and nosetest on appveyor/travis if I ever want my tests to pass again.

Unfortunately, yes.

It is nice to hear that there is a demand in pytest, and I am sorry for my slowness.
I am close to finish with pytest support, but currently there is a major issue in matplotlib that prevents running pytest with xdist (I have not opened issue for it yet, but I am working on solution).
Anyway I think nose should live for some time with pytest until we are sure that there are no false positives.

You can rebase on top of #6730 for pytest support (but it is a bit out of sync, and you need to remove -n $PROC from .travis.yml to disable xdist).

Owner

tacaswell commented Aug 4, 2016

I would like to keep this PR mergeable independent of the pytest PR

@Kojoley You are making more progress on this in the last few weeks than we have made in the last ~ year

tacaswell added this to the 2.1 (next point release) milestone Aug 4, 2016

Member

story645 commented Aug 4, 2016 edited

then what should I do? I'd rather not have to rewrite all my tests only to rewrite them again, but travis and appveyor won't work with them.

@Kojoley does it make sense to factor out just the py.test & nosetest can both run stuff out of your PR into a separate pull request? I don't want to step on your toes/your PR, but it also may make it easier for other people to adopt py.test in their testing and that not have to wait on full py.test conversion.

Owner

tacaswell commented Aug 4, 2016

maybe pull the tests out, merge this to master with no CI (but running the tests locally), and the put a PR on top of #6730 adding the tests there?

Member

story645 commented Aug 4, 2016

Ugh, 'specially as this will only get worse as I write more tests and my coverage will sink really badly. I really think a relatively small PR that just lets nose and pytest work together might be a better solution unless @Kojoley has an argument against.

Member

Kojoley commented Aug 4, 2016

maybe pull the tests out, merge this to master with no CI (but running the tests locally), and the put a PR on top of #6730 adding the tests there?

I agree. We can comment out this line or make a new list of pytest only tests.

my coverage will sink really badly

You can still look at the coverage locally.

I don't want to step on your toes/your PR, but it also may make it easier for other people to adopt py.test in their testing and that not have to wait on full py.test conversion.

My primary goal is to make current test suite both nose & pytest compatible with minimal changes. Just do not use raise KnownFailureTest (I should separate PR for knownfailed function from #6730) and it will work with pytest without changes.

Member

story645 commented Aug 4, 2016

or make a new list of pytest only tests.
I vote for doing that. Also I really really don't want to have to separate my tests from my code. So is their go ahead from you to spin off a PR just for running both nose and py.test?

Just do not use raise KnownFailureTest
already doing that

Member

Kojoley commented Aug 4, 2016

So is their go ahead from you to spin off a PR just for running both nose and py.test?

It will be in the same PR. I have just temporary disabled nose because otherwise travis and appveyor will run hundred times and spend hours of my life, forcing me to wait for nose while the results does not matter, because all the places that touches nose already tested many times and I want simply know is it all right with pytest or not.

Member

story645 commented Aug 4, 2016 edited

It will be in the same PR.

I think we may be on different pages here. I'm proposing really small PR that just adds support for py.test (nose can still do it's thing) that can hopefully be merged into master fairly quickly. My reason for proposing it is lets people like me write py.test code without having to wait on the full conversion PR getting merged in.

Member

Kojoley commented Aug 4, 2016

without having to wait on the full conversion PR getting merged in

PR #6730 is only about pytest compatibility with current test suit. It will not growth beyond that. The full conversion is something that I do not expect in the near future.

I think we can do following thing:

  • I will prepare #6730 ASAP with just single-threaded support and pytest-only tests filter implementation
  • You will rebase this PR on top of it just for check
  • If all is ok, merge #6730 to master, rebase this PR on top of master and merge

But I should warn you about unexpected problems. I really hope #6899 is the last stopper for pytest, but I cannot guarantee this. So if you can wait until weekend with this PR I think it will be the best choice.

Member

story645 commented Aug 4, 2016

Sounds good to me, and can totally wait on the weekend. Thanks

story645 referenced this pull request Aug 10, 2016

Open

[WIP] Categorical Color Mapping #6934

1 of 5 tasks complete
Owner

tacaswell commented Aug 21, 2016

@story645 What is going on with this branch? This mostly rebases cleanly on to current master (see the category branch on my fork).

@tacaswell tacaswell commented on the diff Aug 21, 2016

@@ -4,7 +4,7 @@
# and then run "tox" from this directory.
[tox]
-envlist = py26, py27, py31, py32
@tacaswell

tacaswell Aug 21, 2016

Owner

As long as you are touching this, can you remove 2.6, 3.1 and 3.2 and add 3.4?

Member

story645 commented Aug 21, 2016

Waiting on #6730 , or does #6920 supercede that @Kojoley

Member

Kojoley commented Aug 21, 2016

No, #6920 does not supercede #6730.

Member

Kojoley commented Aug 21, 2016

@story645 please remove the test from default_test_modules var.

Member

story645 commented Aug 23, 2016

Passes travis mostly 'cause this new code is no longer tested as part of this PR. But, all these tests are in #6934 anyway.

@tacaswell tacaswell and 1 other commented on an outdated diff Aug 23, 2016

@@ -1,4 +1,5 @@
Adrien F. Vincent <vincent.adrien@gmail.com>
+<<<<<<< 3d31c694aa6b77b9d8b7b1897f3a67f5be1a54ea
@tacaswell

tacaswell Aug 23, 2016

Owner

merge conflict flags

@story645

story645 Aug 23, 2016

Member

Ugh, it lied and told me there were no conflicts. fixed now.

tacaswell closed this Aug 23, 2016

tacaswell reopened this Aug 23, 2016

@tacaswell tacaswell added needs_review and removed needs_review labels Aug 23, 2016

Owner

tacaswell commented Aug 23, 2016

@story645 this needs a rebase (again). I suspect it is in the travis config files.

@story645 story645 py.test, updating axis ticks, and unitData as class, removed test_cat…
…egory from init, and updated tox
f24330d
Member

story645 commented Aug 23, 2016

Yup, travis and appveyor and the like 'cause of #6730, updated now.

Member

story645 commented Aug 23, 2016

Pretty sure coverage failure isn't my fault since coverage is up/neutral on the files that are part of this PR.

Member

Kojoley commented Aug 24, 2016

I have opened a PR #6974 for the issue

@tacaswell tacaswell merged commit ee8f448 into matplotlib:master Aug 25, 2016

2 of 3 checks passed

coverage/coveralls Coverage decreased (-9.2%) to 61.114%
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

tacaswell removed the needs_review label Aug 25, 2016

story645 deleted the story645:category branch Mar 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment