-
Notifications
You must be signed in to change notification settings - Fork 171
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
[BUG] Force softmax
returning Series
#1139
Conversation
@@ -124,7 +124,8 @@ def softmax(s: pd.Series) -> pd.Series: | |||
:param s: Input Series. | |||
:return: Transformed Series. | |||
""" | |||
return scipy_softmax(s) | |||
|
|||
return pd.Series(scipy_softmax(s), index=s.index, name=s.name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I'd like use np.exp(s) / sum(np.exp(s))
instead of using scipy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the SciPy implementation comes with numerical stability checks for free. Is there a reason why you would prefer to use a NumPy implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To totally replace scipy. Since we only use it in math.py
and functions/impute.py
mode
function infunctions/impute.py
could replace bySeries.value_counts
math.py
sigmoid
(expit
) could usenp.exp
softmax
(scipy.special.softmax
) could usenp.exp
logit
(scipy.special.softmax
) could usenp.log
normal_cdf
(scipy.stats.norm.cdf
) andprobit
(scipy.stats.norm.ppf
) can't replace easily.
Codecov Report
@@ Coverage Diff @@
## dev #1139 +/- ##
==========================================
- Coverage 97.36% 97.34% -0.02%
==========================================
Files 77 77
Lines 3186 3240 +54
==========================================
+ Hits 3102 3154 +52
- Misses 84 86 +2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am good to go. I think we should keep the SciPy soft Mac implementation rather than switch to numpy, for numerical stability reasons (IIRC).
PR Description
Please describe the changes proposed in the pull request:
This PR resolves https://github.com/pyjanitor-devs/pyjanitor/runs/7703616556?check_suite_focus=true .
PR Checklist
Please ensure that you have done the following:
<your_username>
:dev
, but rather from<your_username>
:<feature-branch_name>
.AUTHORS.md
.CHANGELOG.md
under the latest version header (i.e. the one that is "on deck") describing the contribution.Automatic checks
There will be automatic checks run on the PR. These include:
Relevant Reviewers
Please tag maintainers to review.