Skip to content

Remove finance module #7071

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 4 commits into from

Conversation

jenshnielsen
Copy link
Member

And replace with a shim using mpl_finance. I would suggest backporting to 2.0

@jenshnielsen
Copy link
Member Author

This probably need an api change note

@tacaswell
Copy link
Member

This is probably worth a note to the user and devel lists one more time.

This is also going to require coordination with the major down-stream packagers (fedora, debian, arch, enthought, contiuum, homebrew, macports, pythonxy, winpython) that they may need to create a new package.

I am 50/50 on backporting to 2.0. No compelling argument against other than making sure there is enough time for the transition to go smoothly. Can definitely be talked into it.

@tacaswell tacaswell added this to the 2.1 (next point release) milestone Sep 9, 2016
@jenshnielsen
Copy link
Member Author

I honestly hoped that we could avoid waisting to many package maintainers time on this package since it's a pure python package with only one file but we should probably send them a heads up anyway.

My main reason to suggest 2.0 was that we already backported the removal of the examples.
For ease of communication I would prefer that we could write a api_changes note that said
something like:

matplotlib.finance and it's accompanying examples has been split out into a separate package 
see ...

@NelleV
Copy link
Member

NelleV commented Sep 10, 2016

I agree with @tacaswell that we may want to wait until 2.1 and not backporting 2.0 to make the transition smoothly.

@jenshnielsen
Copy link
Member Author

In my opinion that would be the very definition of non smooth. You were not willing to disable the examples so that they could be removed along with the module so they have already been removed from 2.0 thus the module should be removed along with the examples anything else just raises extra confusion for all users

@NelleV
Copy link
Member

NelleV commented Sep 11, 2016

Right now there is no deprecation warning when importing the finance module.
IMO, you need at least two releases of deprecation before removing a function or a module. It would be really bad to remove a module without prior warning.

The documentation examples (FYI, mostly unrelated to the finance module) do not need to follow the same deprecation cycle, as no users uses those as dependencies.

In additions the examples were not running… If they hadn't caused any problem, I wouldn't have encouraged removing them (note that api.yahoo is still either down or insanely slow).

@jenshnielsen
Copy link
Member Author

The examples were running and still are. Travis ran them 6 hours before your issue and I ran them again when I created the new package afterwards. I guess they failed for you due to a temporary issue on yahoo's site or a proxy issue

@jenshnielsen
Copy link
Member Author

or more likely your have been blocked by a rate limiter on the yahoo api since you have been rebuilding over and over again

@NelleV
Copy link
Member

NelleV commented Sep 12, 2016

Your interpretation on why we couldn't build the documentation in Reading anymore is probably correct. Considering I had to rebuild the documentation many times in order to have the list of dependencies, that may have been a problem. It is surprising that no one else had this problem before: I only rebuild the documentation around 20 times, which is a reasonable amount of time for anyone working on the documentation.

Anyhow, that doesn't changes the fact that removing the finance module should go through a proper deprecation cycle. It should raise a Deprecation Warning for at least two releases before being removed. Right now, it is only documented in the docstring. It is unlikely anyone noticed this deprecation.

This module is deprecated in 1.4 and will be moved to `mpl_toolkits`
or it's own project in the future.

Shim to provide backword compatibility for finance
Copy link
Contributor

Choose a reason for hiding this comment

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

"backward"

@anntzer
Copy link
Contributor

anntzer commented Oct 15, 2016

Given the lack of agreement, perhaps we can just noisily deprecate this module for 2.0 and remove it (without introducing an optional dependency on mpl_finance -- let's just get rid of it) in 2.1? With some luck(?) someone who actually uses this module (which seems of interest for none of us) will notice the warning, come forward and propose to maintain it, possibly somewhere else.

@NelleV I don't know why you'd want a deprecation cycle of at least two releases. Since 1.1 mpl has averaged one release (x.y) per year (in fact I just noticed that each 1.x.0 release, except for 1.0, occured in year 201x :-)) so that seems a ridiculously long cycle...

@NelleV
Copy link
Member

NelleV commented Oct 15, 2016

@anntzer Because that's the norm. It is normal to have long deprecation cycles for a project of the size of matplotlib where there are tens of thousands of users. Some would even argue that you should just not break backward compatibility.
Backward compatibility has always been a huge part of matplotlib's culture. It took us two years to even decide to change the defaults, and two extra years to implement those defaults. Here, we are talking about removing a whole module. That should not be taken lightly IMO.

Second, maybe we should try to have shorter release cycles :)
That's hard for 2.0.0 because of all the new features introduced by the new defaults, and all the border effects, but it should get easier.

@anntzer
Copy link
Contributor

anntzer commented Oct 15, 2016

I think this kind of deprecation policy is a bit misguided; at the end of the day what matters most for the end user is not the number of minor releases but the physical duration of the deprecation cycle. From his point of view, it's basically "let's run this code, as usual... oh there's a deprecation warning, I need to either update the code, or to stick everything in a venv/conda env with pinned dependencies". (Additionally, these days conda envs are really easy to set up. Back when this was not an option, I could understand the need to keep things more stable.) The duration of the deprecation cycle basically tells you how much time he has to do this. Honestly, if a year (or six months, assuming we move to a faster release cycle, which I don't feel strongly about at all) are not enough for someone to move his codebase into a conda env, I don't see how givin him two years will change anything.

One thing I do agree, though, is that you do want at least one cycle of noisy deprecation. (As a side note, I remember ranting with you about how I hated that the IPython project (pre big-split) was often breaking their (semi-internal, I guess) API -- but I'd say that was before conda envs were really common, and due to the lack of noisy deprecations.) In fact we could even make the deprecation message more explicit: "Due to lack of active maintenance, the XYZ module is scheduled for removal in a future version. Interested parties are invited to step forward to maintain the module." Then we'll see if anyone actually cares...

I know very well that the 2.0 cycle is rediculously long because of the many difficulties related to the default style change. I don't actually mind the relatively slow release cycle -- I am happy that things are done with the proper level of attention for details.

@NelleV
Copy link
Member

NelleV commented Oct 16, 2016

@anntzer When I wrote my phd thesis, I ran code that I wrote 3 years before that and hadn't touched since. 1 year is not that common.

Some people don't use Conda. To quote a visiting scholar from BIDS, "apart in the scientific python community, I don't know anyone who uses conda". More exactly, he said that before coming here to visit the Berkeley Institute for data science, he had never heard of conda.
matplotlib reaches far beyond the scientific python community. I wouldn't be surprised that our vast majority of users don't use conda and don't even know about conda :)

Anyhow, I don't feel strongly about the 2 deprecation cycle, thought I do think that strong backward compatibility is what makes matplotlib such a widely used package.

@anntzer
Copy link
Contributor

anntzer commented Oct 16, 2016

But then your position amounts to saying that we should never deprecate anything, because even a one-year (or, in your case, three-year!) deprecation cycle can catch people unprepared. For example, even though we are trying as hard as possible to make 2.0 "backwards-compatible" in the sense that code that ran on 1.5 will still produce a somewhat similar plot on 2.0, I think it's clear that many plots will require some amount of tuning to stay aesthetically pleasing. And there's nothing we can do about it... Does that mean that, because you wanted your three-year old code to run unmodified after the switch to 2.0, we cannot make that change?

Now, I don't know what is the actual status of the finance module in terms of code quality, but I don't think that keeping half (and I'm being generous there)-working code in the codebase such as mpl_toolkits.gtktools (#6554) or some parts of mlab (#6577) is helpful for anyone either.

@NelleV
Copy link
Member

NelleV commented Oct 16, 2016

I am saying these things should not be taken lightly.

@NelleV
Copy link
Member

NelleV commented Oct 16, 2016

I am slightly confused on how a two release cycle of deprecation bothers you though. It's not like its code you'll have to maintain. Before we tagged which version we removed deprecated stuff on sklearn to easily grep them and remove them before the release, some functions were deprecated for a very long time: no one noticed.

@anntzer
Copy link
Contributor

anntzer commented Oct 16, 2016

I agree code cannot be removed randomly and that proper care is needed. In fact that's exactly why I proposed a middle ground: deprecate in 2.0 (I agree with you that the deprecation-in-doc-only of the finance module is insufficient), and remove after that.

It's not code I have to maintain, but as long as it's there, people will notice issues, open PRs, etc. and energy will be lost on that (even though I agree it's not that much). In fact, searching for "finance" in the issues led me to your own comment (#7057 (comment)):

We also need to stop keeping code stacked, that no one uses, as it leads to situation were we have code that doesn't run, without anyone noticing for years and years and years.

:-)

@ivanov
Copy link
Member

ivanov commented Oct 19, 2016

Seems like #7295 took care of issuing a deprecation warning, so this won't be removed until 2.2 - Close me, maybe? ;)

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.

7 participants