Skip to content
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

something like this #311

Merged
merged 11 commits into from
Nov 2, 2023
Merged

Conversation

Groni3000
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

Groni3000 commented Oct 31, 2023

@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
Copy link
Contributor Author

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

@Groni3000
Copy link
Contributor Author

Groni3000 commented Nov 1, 2023

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

@bukosabino bukosabino merged commit 6d124a7 into bukosabino:master Nov 2, 2023
@bukosabino bukosabino mentioned this pull request Nov 2, 2023
@bukosabino
Copy link
Owner

Hi @Groni3000,

Congratulations on your work!

Your problem was related to linters (isort and pylint). I have solved it for you: #330

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

@Groni3000
Copy link
Contributor Author

@bukosabino, thank you very much!

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.

2 participants