Skip to content

DOC: explain EWM #34901

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 9 commits into from
Closed

DOC: explain EWM #34901

wants to merge 9 commits into from

Conversation

KrishnaSai2020
Copy link
Contributor

@pep8speaks
Copy link

pep8speaks commented Jun 20, 2020 •
edited
Loading

Hello @KrishnaSai2020! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-06-20 14:03:50 UTC

@KrishnaSai2020
Copy link
Contributor Author

right I'll fix that pep 8 issue and reopen this pr

@charlesdong1991
Copy link
Member

thanks for the PR @KrishnaSai2020

can you pls fix it though? Not sure about what the previous contributor did, but unlikely his/her PR got merged with the PEP8 issue.

Looks like your expanded EM to Exponential Weighted caused it.

@KrishnaSai2020
Copy link
Contributor Author

Ah I see! should i close this, fix and reopen or is there an easier way to edit the PR??

@jreback jreback changed the title Fixed docs issue #34867 DOC: explain EWM Jun 20, 2020
@charlesdong1991
Copy link
Member

you can fix it in this PR, no need to close it and reopen a new one, thanks!

@charlesdong1991 charlesdong1991 added Docs Window rolling, ewma, expanding labels Jun 20, 2020
jreback
jreback requested changes Jun 20, 2020
View reviewed changes
pandas/core/generic.py Outdated
@@ -10460,7 +10460,7 @@ def _add_series_or_dataframe_operations(cls):
Add the series or dataframe only operations to the cls; evaluate
the doc strings again.
"""
from pandas.core.window import EWM, Expanding, Rolling, Window
from pandas.core.window import EWF, Expanding, Rolling, Window
Copy link
Contributor

Choose a reason for hiding this comment

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

i would be ok with actually writing this classname out if we are going to change it, e.g. ExponentialMoving or ExponentialMovingWindow

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good might as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

quick one could you clarify what you mean by writing it out?

Copy link
Member

@charlesdong1991 charlesdong1991 Jun 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think @jreback meant you could rename EWM to ExponentialMoving or ExponentialMovingWindow in core.window.ewm.py where the class is created

Copy link
Contributor Author

Choose a reason for hiding this comment

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

k

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changes resolved

@KrishnaSai2020
fix pep 8 issues
c165fc3 
reduced line length
@KrishnaSai2020
Copy link
Contributor Author

@charlesdong1991 fixed the pep 8 problem

KrishnaSai2020
KrishnaSai2020 commented Jun 20, 2020
View reviewed changes
Copy link
Contributor Author

@KrishnaSai2020 KrishnaSai2020 left a comment

Choose a reason for hiding this comment

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

please re-review

charlesdong1991
charlesdong1991 reviewed Jun 20, 2020
View reviewed changes
pandas/core/window/ewf.py Outdated
@@ -59,11 +59,11 @@ def get_center_of_mass(
return float(comass)


class EWM(_Rolling):
class Exponential_moving(_Rolling):
Copy link
Member

Choose a reason for hiding this comment

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

could you use ExpontentialMoving other than Expontential_moving?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure

@charlesdong1991
Copy link
Member

I think what was meant by @jreback is to rename EWM to ExponentialMoving or ExpontentialMovingWindow other than to EWF.

so you might also need to replace all your EWF to the new class name you choose.

@KrishnaSai2020
Copy link
Contributor Author

@charlesdong1991 I think that's all instances refactored let me know if there more.

@KrishnaSai2020 KrishnaSai2020 requested a review from jreback June 20, 2020 14:06
charlesdong1991
charlesdong1991 reviewed Jun 20, 2020
View reviewed changes
Copy link
Member

@charlesdong1991 charlesdong1991 left a comment

Choose a reason for hiding this comment

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

thanks, several comments

pandas/core/generic.py
@@ -10460,7 +10460,8 @@ def _add_series_or_dataframe_operations(cls):
Add the series or dataframe only operations to the cls; evaluate
the doc strings again.
"""
from pandas.core.window import EWM, Expanding, Rolling, Window
from pandas.core.window import exponentialmoving as em
Copy link
Member

@charlesdong1991 charlesdong1991 Jun 20, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

i don't think is necessary now to rename the file from ewm to expontentialmoving, but @jreback could comment here.

pandas/core/generic.py
@@ -10520,7 +10521,7 @@ def ewm(
axis=0,
):
axis = self._get_axis_number(axis)
return EWM(
return EWF(
Copy link
Member

Choose a reason for hiding this comment

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

still need to change?

pandas/core/generic.py
@@ -10532,7 +10533,7 @@ def ewm(
axis=axis,
)

cls.ewm = ewm
cls.eem = em
Copy link
Member

Choose a reason for hiding this comment

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

emm, why the change here?

pandas/core/window/__init__.py
@@ -1,3 +1,3 @@
from pandas.core.window.ewm import EWM # noqa:F401
from pandas.core.window.ewf import exponentialmoving # noqa:F401
Copy link
Member

Choose a reason for hiding this comment

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

is there still ewf file?

pandas/core/window/exponentialmoving.py
@@ -59,11 +59,11 @@ def get_center_of_mass(
return float(comass)


class EWM(_Rolling):
class Exponentialmoving(_Rolling):
Copy link
Member

Choose a reason for hiding this comment

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

i think they might prefer capital letter for Moving

pandas/core/window/exponentialmoving.py
@@ -185,7 +185,7 @@ def __init__(

@property
def _constructor(self):
return EWM
return EWF
Copy link
Member

Choose a reason for hiding this comment

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

this still need to change too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tell you what mate give me some time I'll rollback all the changes, start afresh and create a new pull request. i've confused myself. thanks for your help!

jreback
jreback requested changes Jun 20, 2020
View reviewed changes
pandas/core/generic.py
@@ -10460,7 +10460,8 @@ def _add_series_or_dataframe_operations(cls):
Add the series or dataframe only operations to the cls; evaluate
the doc strings again.
"""
from pandas.core.window import EWM, Expanding, Rolling, Window
from pandas.core.window import exponentialmoving as em
Copy link
Contributor

Choose a reason for hiding this comment

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

use python naming conventions, e.g. ExponentialMoving, and don't abbreviate as this is a class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

k thanks i'll keep that in mind

@KrishnaSai2020
Copy link
Contributor Author

#34910

@MarcoGorelli MarcoGorelli mentioned this pull request Jun 20, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@charlesdong1991 charlesdong1991 charlesdong1991 left review comments

@jreback jreback jreback requested changes

Assignees
No one assigned
Labels
Docs Window rolling, ewma, expanding
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

DOC: pandas/pandas/core/window/ewm.py
4 participants
@KrishnaSai2020 @pep8speaks @charlesdong1991 @jreback