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

EVERYONE: Removing the 'fillna' keyword option if there is no opposition! #780

Open
twopirllc opened this issue Mar 25, 2024 · 3 comments
Open
Labels
enhancement New feature or request info Informational

Comments

@twopirllc
Copy link
Owner

Hello All,

First of all, thanks for using Pandas TA!

It would not have made it this far along without the generous help of those that have contributed in many ways. Unfortunately there is still a lot that needs to be accomplished before main is updated. If you are looking for immediate improvements or found some bugs in v0.3.14b, please try the development version first.

$ pip install -U git+https://github.com/twopirllc/pandas-ta.git@development

Anyhow, this is a notice that the keyword 'fillna' in each indicator will be removed since it is likely unused code and just wasting space. If you use it, please provide a 👍🏼 otherwise a 👎🏼. Feel free to add more detail of why it should be kept if you use it.

For Pandas TA to endure, please consider submitting a donation or sponsoring via GitHub or Buy me a Coffee.

Thank you ❤️

Note: Any unrelated issues or comments with this topic will be deleted. If you have other Issues, open them here.

@twopirllc twopirllc added enhancement New feature or request info Informational labels Mar 25, 2024
@twopirllc
Copy link
Owner Author

Hey @luisbarrancos

Am I wrong in assuming that is easier to 'fillna' in one go in the post processing phase after applying indicators than handling each indicator individually?

KJ

@luisbarrancos
Copy link
Contributor

luisbarrancos commented Jun 10, 2024

@twopirllc Out of curiosity, is this concern arising primarily due to changes in Pandas>2.1 related to the fillna method, to be replaced by ffill() and bfill() methods?

I'm actually reconsidering my vote.
On one side we have a global post-process fillna (assuming there was a initial time-series pass to avoid garbage) which simplifies the code and ensures consistency.
This global pass might even assist in revealing unexpected behavior of some indicators, specially more complex indicators, that might not deal as solidly with corner cases. Chaining indicators would also benefit from a single logic for NaNs, and these were my main points in favor of a global fillna, hence removal of the parameter.

On the other side, we do have a bit of conundrum in that some indicators might require different fill methods and different platforms might expect different initializations. That is the responsibility of whomever writes the indicators. However, if your indicator relies on previous indicators, such as a SMA or EMA, you might need to override locally the initialization in order to match some platform, or duplicate code.
For example, in order to provide a behavior matching the published peer-reviewed literature, if it exists, and a popular platform, such as Trading View.
I'm thinking of SMA and EMAs since they are usually building blocks, and you back fill SMAs rather than zero fill, while EMAs and other weighted MAs are more sensitive to recent values and require forward fills.

The TMO, just to name an example, would benefit from forward fills, but then again, it should not produce NaNs if given clean time-series, with the exception of course, of requiring N data points to produce the 1st value.
Someone chaining the TMO to subsequent indicators, however, might want to set forward fill if there were insufficient data points to begin with. But they can always call the bfill() or ffill() method in the DataFrame storing the TMO, before proceeding with the rest of their own indicator.
So there's that in favor of removing fillna and the reason for my vote for removal.

Whomever writes the indicator might prefer, to avoid code duplication, just call existing indicators and change the timeseries initialization in the call itself. This is advantageous and avoids code duplication. But in practice I'm not sure we're going to be seeing this done, outside of the SMA, EMA and other weighted moving averages that might require different initializations.

I fear I might've brought more doubts into the discussion, rather than enlightenment, but overall simplicity would be best, so removal of the fillna would still remain my vote. If there is a localized need to change a fill, probably this is going to be rare enough to justify duplicating code, in the worst case.

@twopirllc
Copy link
Owner Author

Hi @luisbarrancos,

I fear I might've brought more doubts into the discussion, rather than enlightenment

🤣 But really, thanks for commenting back with some excellent points on both sides of the spectrum.

so removal of the fillna would still remain my vote

My current plan is to remove them before updating main, which will be awhile unfortunately, so there is still some time for other's opinions for the case of keeping them.

Thanks for the feedback

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request info Informational
Projects
None yet
Development

No branches or pull requests

2 participants