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

feat: Added pctChange() to DataFrame and divNoNan() to $MathOps in DataFrame #418

Merged
merged 11 commits into from
Mar 9, 2022

Conversation

NeonSpork
Copy link
Contributor

@NeonSpork NeonSpork commented Mar 8, 2022

Structure is similar to diff() function, attempted to emulate the functionality from pct_change() function from pandas.

Chose to use camelcase naming here, thusly naming it pctChange().

Added function to DataFrame interface.

Added divNoNan() from the tensorflow.js math operators to $MathOps. This was necessary to emulate the functionality from pandas, such that dividing by zero resulted in 0 not Infinity. Felt that this was a good addition regardless, seeing as it is one of the baseline math operators provided by tensorflow.js itself. As such it was trivial to add. Tensorflow docs here: tf.divNoNan()

Added testing. Tests technically work since they return the correct values, but the tests fail since JS is a little bit strange about floating point division.

A possible workaround is to choose calculations that will pass tests, since functionally pctChange() is working as expected.

NeonSpork and others added 6 commits March 8, 2022 09:24
Diff port into working branch for pct_change
Floating points are weird in JS. Will likely need refactor to make tests
pass since _functionally_ they work.
@NeonSpork NeonSpork changed the title feat: Port of pct_change() from pandas feat: Added pctChange() to DataFrame Mar 8, 2022
@NeonSpork NeonSpork changed the title feat: Added pctChange() to DataFrame feat: Added pctChange() to DataFrame and divNoNan() to $MathOps in DataFrame Mar 8, 2022
@NeonSpork
Copy link
Contributor Author

@risenW I can't quite seem to get the TensorFlow functions div() or divNoNan(). Since the actual functionality is working, I propose maybe working around it by choosing to use less wonky math in the tests?

Wonkiness of slight difference in calculation (`0.80000000001234` instead
of `0.8` for example) is due to JavaScript.

Functionally the tests worked, so I chose divisions that JavaScript does reliably.
@NeonSpork
Copy link
Contributor Author

Worked around JavaScript's wonkiness in the tests by using divisions that perform more reliably.

@NeonSpork
Copy link
Contributor Author

Resolves #383 in conjunction with PR #414

@risenW
Copy link
Member

risenW commented Mar 8, 2022

Resolves #383 in conjunction with PR #414

Thanks, will review and test ASAP

@risenW risenW self-requested a review March 8, 2022 11:12
@NeonSpork
Copy link
Contributor Author

Added unit tests for danfojs-browser

@risenW
Copy link
Member

risenW commented Mar 9, 2022

@NeonSpork I just merged the diff PR, and there's a minor conflict here. Can you fix it, or do you want me to?

@NeonSpork
Copy link
Contributor Author

@NeonSpork I just merged the diff PR, and there's a minor conflict here. Can you fix it, or do you want me to?

Yea the merge conflicts are likely due to me adding the tests in the same spot in the test! I'll go through it and check right away

@risenW risenW merged commit f49f16c into javascriptdata:dev Mar 9, 2022
@NeonSpork
Copy link
Contributor Author

Conflicts resolved and checks passed! 👍

@NeonSpork NeonSpork deleted the pct_change-port branch March 9, 2022 07:51
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