bukosabino / ta

Technical Analysis Library using Pandas and Numpy
https://technical-analysis-library-in-python.readthedocs.io/en/latest/
MIT License
4.25k stars 867 forks source link

something like this #311

Closed Groni3000 closed 10 months ago

Groni3000 commented 1 year ago

I fixed this indicator. It was completely wrong. Important things:

  1. Now it returns Series with NaN values by default unless new optional method parameter dropnans is not True OR you can specify self._check_fillna(self._vpt, value=-1) instead of value=0 to make it bfill, but I would not recommend it. Therefore it returns self._vpt with property self._vpt.shape[0] <= self.close.shape[0].
  2. Provided new optional method parameter smoothing_factor like in TradingView implementation of this indicator. But it's not used by default (like in pure definition of this indicator).
  3. Added some type|value checking, feel free to delete it. I added them to feel safe and didn't find a place with checkers.
  4. I didnt'find a place to add important comment about shifting and fill_value with series mean, so I did it in 273 line... Not in right place but ... whatever. It should be deleted, I guess.
Groni3000 commented 1 year ago

test_py37 Errors with Imports are incorrectly sorted and/or formatted ... That's cuz of from typing import Union I guess ... Dunno how to fix without just deleting it and type hint.

coverage Errors cuz of new nans behaviour I guess... Well, This new behaviour is correct one, so ... You can just delete everything, but use this CORRECT indicator def _run(self): self._vpt = (self._closes.pct_change() * self._volume).cumsum()

That's main thing

Groni3000 commented 1 year ago

https://app.circleci.com/pipelines/github/bukosabino/ta/382/workflows/1c05ecdd-31f3-4532-88f0-c1c8c4106817/jobs/964?invite=true#step-103-16

seems like pylint error, not my.

Well, I tried to do it in ta style, but again, I don't think that we can fill NaN values with mean... That seems so wrong to me...

bukosabino commented 10 months ago

Hey @Groni3000,

Thanks for your efforts on this indicator.

Don't worry about the linter bugs.

Can you add some tests to the code?

Groni3000 commented 10 months ago

@bukosabino, sorry it took so long, didn't have time at all.

Glanced into your test folder, tried to make tests in your style. Didn't even try to understand what's going on in integration folder, seems like it's supposed to be automated. In unit tests I added new class and some tests for unsmoothed and smoothed versions. Though I didn't really understood why are you doing two tests... Function indicator (1st test) creates class Indicator and runs method, 2nd test uses created class Indicator and test just runs method... Seems like duplicated test for me.

And I changed VPT as I saw it in definitions mentioned in links (added them too in docs, I tried to write briefly and informatively).

Hope it will be helpful!

P.S. What is test_py37 at all? CircleCI doesn't even open for me xDD Maybe I'm supposed to sign in first... Though I dont think I've created acc there long time ago, but still was able to read what kind of error it is.

Groni3000 commented 10 months ago

And, btw, I couldn't find anything about VPT (or PVT on TradingView) on https://stockcharts.com/ to insert it in docs.

Groni3000 commented 10 months ago

Embarrassing for me =) Lsp doesn't work and I was coding at 2 AM — these are my excuses xDD

bukosabino commented 10 months ago

Hi @Groni3000,

Congratulations on your work!

Your problem was related to linters (isort and pylint). I have solved it for you: https://github.com/bukosabino/ta/pull/330/

The volume price trend indicator is now fixed and it is included in the latest version v0.11.0.

Groni3000 commented 10 months ago

@bukosabino, thank you very much!