[ENH] Log normal distribution #22 #214

bhavikar04 · 2024-03-15T16:28:34Z

Reference Issues/PRs

#22

What does this implement/fix? Explain your changes.

Implemented a log normal probability distribution

Does your contribution introduce a new dependency? If yes, which one?

None

What should a reviewer concentrate their feedback on?

-energy method

Did you add any tests for the change?

no

Any other comments?

PR checklist

For all contributions

I've added myself to the list of contributors with any new badges I've earned :-)
How to: add yourself to the all-contributors file in the skpro root directory (not the CONTRIBUTORS.md). Common badges: code - fixing a bug, or adding code logic. doc - writing or improving documentation or docstrings. bug - reporting or diagnosing a bug (get this plus code if you also fixed the bug in the PR).maintenance - CI, test framework, release.
See here for full badge reference
[x ] The PR title starts with either [ENH], [MNT], [DOC], or [BUG]. [BUG] - bugfix, [MNT] - CI, test framework, [ENH] - adding or improving code, [DOC] - writing or improving documentation or docstrings.

For new estimators

I've added the estimator to the API reference - in docs/source/api_reference/taskname.rst, follow the pattern.
I've added one or more illustrative usage examples to the docstring, in a pydocstyle compliant Examples section.
If the estimator relies on a soft dependency, I've set the python_dependencies tag and ensured
dependency isolation, see the estimator dependencies guide.

fkiraly

Nice!

A few things about mechanics of making PR:

you have committed some pycache files, these should not be tracked. Your .gitignore might not be setup properly?
You are making changes to empirical and laplace too, is that by accident?

bhavikar04 · 2024-03-16T14:48:33Z

Hey,

In laplace I only changed a comment, empirical was by accident. I'll setup the .gitignore properly and make all the necessary changes soon. Apologies for the delay, I appreciate the review.

fkiraly · 2024-03-22T23:36:47Z

skpro/distributions/__init__.py

@@ -3,7 +3,9 @@
 # copyright: skpro developers, BSD-3-Clause License (see LICENSE file)
 # adapted from sktime

-__all__ = ["Laplace", "Normal"]
+__all__ = ["Log-Normal","Empirical", "Laplace", "Normal"]


what state of the repository are you branching off from? This does not look like the most recent version, it looks like it is half a year old. Please make sure you update your fork regularly.

fkiraly

I think there is still an issue with your pull request, you seem to be branching off a much earlier version of the repository. Make sure your fork is up to date, you can do that on GitHub by clicking "sync fork" at the top right.

That's why your changes are being shown as conflicting.
If you update your branch, it will probably throw a lot of merge errors.

I would recommend to update your main, and then start a new branch. Then, move only the lognormal file over, and make changes to __init__ again.

fkiraly · 2024-03-22T23:41:12Z

skpro/distributions/log_normal.py

+            d = self.loc[x.index, x.columns]
+            mu_arr, sd_arr = d._mu, d._sigma
+
+            c_arr = x*(2*self.cdf(x)-1)-2*exp((mu_arr+sd_arr**2)/2)*(self.cdf((np.log(x)-mu_arr-sd_arr**2)/sd_arr)+self.cdf(sd_arr/mu_arr**0.5)-1)


could you kindly write down the formula in math, or explain otherwise how you are getting this expression?

Hey, so this is basically the CRPS score which is E[IX-xI] -0.5E[|X-X'|] from which I was unable to isolate the first term. Wolfram alpha too wasnt able to produce a closed form for the integral. Can we change the description of the energy method accordingly or would you rather we go by the approximation in the base class?

Why don't we try to spend a short discussion on trying to see whether we can get somewhere. Removing it entirely is always an option.

A "trick" to isolate the first term is to observe that adding the same constant to x and X (so, the location parameter) should leave the formula unchanged.

Regarding the integral, which integral concretely are you feeding into Wolfram Alpha?

yes so this is what I got. If I try to add limits it crashes. Am I misinterpreting the integral?

this looks correct. Now you need to add the limits. That should be an easy substitution, no? I recommend, do that manually. Use that

$\lim_{x\rightarrow -\infty} \mbox{erf}(x) = -1$, and $\lim_{x\rightarrow \infty} \mbox{erf}(x) = 1$. You need to be careful with the sign, but that should be it?

The number 0.707 etc should be $\frac{1}{2} \sqrt{2}$, but it doesn't matter for the limits.

moved discussion here: #219

bhavikar04 · 2024-03-23T08:16:16Z

@fkiraly please take a look. I haven't made any changes to empirical.py.

I think there is still an issue with your pull request, you seem to be branching off a much earlier version of the repository. Make sure your fork is up to date, you can do that on GitHub by clicking "sync fork" at the top right.

That's why your changes are being shown as conflicting. If you update your branch, it will probably throw a lot of merge errors.

I would recommend to update your main, and then start a new branch. Then, move only the lognormal file over, and make changes to __init__ again.

okay, I did that. Thank you

fkiraly · 2024-03-23T13:28:54Z

okay, I did that. Thank you

I don't think that has worked, you need to resolve the conflicts:

Here is the GitHub guide on the topic:
https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/resolving-a-merge-conflict-on-github
Let me know if you need any help! We can do a screenshare.

bhavikar04 · 2024-03-23T14:39:38Z

okay, I did that. Thank you

I don't think that has worked, you need to resolve the conflicts:

Here is the GitHub guide on the topic: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/addressing-merge-conflicts/resolving-a-merge-conflict-on-github Let me know if you need any help! We can do a screenshare.

No,no I havent committed the new branch yet, just made the changes locally. Was awaiting your response on the energy method dilemma. Thank you so much

fkiraly · 2024-03-23T20:00:41Z

Was awaiting your response on the energy method dilemma. Thank you so much

Ensuring your PR is up-to-date and non-conflicting is a basic requirement for proper review, so I'd recommend not to wait with anything until you fix that.

bhavikar04 · 2024-03-24T06:01:25Z

Was awaiting your response on the energy method dilemma. Thank you so much

Ensuring your PR is up-to-date and non-conflicting is a basic requirement for proper review, so I'd recommend not to wait with anything until you fix that.

right so I'll make a new PR and close this one.

fkiraly · 2024-03-24T13:05:04Z

Thanks.
This one looks good now: #218

I've moved the content regarding the energy function into this issue: #219

fkiraly and others added 7 commits August 24, 2023 21:38

empirical distr

c8e255a

export, docstring example

b8ad5eb

fix docstring example

6e452fd

Update empirical.py

24c6b2c

Merge branch 'main' into empirical-distr

400a7e7

adding lognormal distribution class based on normal template

5aa46d9

Remove log_normal.py test file

8baf010

fkiraly requested changes Mar 15, 2024

View reviewed changes

bhavikar04 added 2 commits March 23, 2024 03:24

modified gitignore

30add41

remove untracked files

48b26de

fkiraly reviewed Mar 22, 2024

View reviewed changes

fkiraly requested changes Mar 22, 2024

View reviewed changes

fkiraly reviewed Mar 22, 2024

View reviewed changes

fkiraly mentioned this pull request Mar 24, 2024

[ENH] explicit/analytic form of energy function for log-normal distribution #219

Open

fkiraly closed this Mar 24, 2024

fkiraly mentioned this pull request Apr 7, 2024

[ENH] Log-normal probability distribution #218

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ENH] Log normal distribution #22 #214

[ENH] Log normal distribution #22 #214

bhavikar04 commented Mar 15, 2024

fkiraly left a comment

bhavikar04 commented Mar 16, 2024

fkiraly Mar 22, 2024

fkiraly left a comment •

edited

Loading

fkiraly Mar 22, 2024

bhavikar04 Mar 23, 2024

fkiraly Mar 23, 2024

bhavikar04 Mar 23, 2024 •

edited

Loading

fkiraly Mar 23, 2024 •

edited

Loading

fkiraly Mar 24, 2024

bhavikar04 commented Mar 23, 2024

fkiraly commented Mar 23, 2024

bhavikar04 commented Mar 23, 2024

fkiraly commented Mar 23, 2024

bhavikar04 commented Mar 24, 2024

fkiraly commented Mar 24, 2024 •

edited

Loading

[ENH] Log normal distribution #22 #214

[ENH] Log normal distribution #22 #214

Conversation

bhavikar04 commented Mar 15, 2024

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Does your contribution introduce a new dependency? If yes, which one?

What should a reviewer concentrate their feedback on?

Did you add any tests for the change?

Any other comments?

PR checklist

For all contributions

For new estimators

fkiraly left a comment

Choose a reason for hiding this comment

bhavikar04 commented Mar 16, 2024

fkiraly Mar 22, 2024

Choose a reason for hiding this comment

fkiraly left a comment • edited Loading

Choose a reason for hiding this comment

fkiraly Mar 22, 2024

Choose a reason for hiding this comment

bhavikar04 Mar 23, 2024

Choose a reason for hiding this comment

fkiraly Mar 23, 2024

Choose a reason for hiding this comment

bhavikar04 Mar 23, 2024 • edited Loading

Choose a reason for hiding this comment

fkiraly Mar 23, 2024 • edited Loading

Choose a reason for hiding this comment

fkiraly Mar 24, 2024

Choose a reason for hiding this comment

bhavikar04 commented Mar 23, 2024

fkiraly commented Mar 23, 2024

bhavikar04 commented Mar 23, 2024

fkiraly commented Mar 23, 2024

bhavikar04 commented Mar 24, 2024

fkiraly commented Mar 24, 2024 • edited Loading

fkiraly left a comment •

edited

Loading

bhavikar04 Mar 23, 2024 •

edited

Loading

fkiraly Mar 23, 2024 •

edited

Loading

fkiraly commented Mar 24, 2024 •

edited

Loading