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

weighted KDE returns nan #56

Closed
Clownshift opened this issue Oct 1, 2021 · 5 comments · Fixed by #62
Closed

weighted KDE returns nan #56

Clownshift opened this issue Oct 1, 2021 · 5 comments · Fixed by #62
Assignees
Labels
bug Something isn't working

Comments

@Clownshift
Copy link
Collaborator

The example0-notebook is actually broken:
I don't know since when, but if weights are given, the kde returns an array full of nan as kde.pdf

To be clear: I believe that this is a problem in the code not in the example.

Not sure exactly what the actual problem is. I will investigate in the next few days (no time today)

@Clownshift Clownshift added the bug Something isn't working label Oct 1, 2021
@Clownshift Clownshift self-assigned this Oct 1, 2021
@Clownshift
Copy link
Collaborator Author

Still investigating, where exactly the issue comes from...

@jokr91 Do you have a hunch why normalized weights might lead to an issue and non-normalized weights seem to not cause the issue?
(There is no normalization in the python frontend. Afaik, the cpp backend takes care of that. Not sure, if this is generally done, or whether it is being checked, if the weights are already normalized eventually)

Screenshot from 2021-10-01 14-27-43

@Clownshift
Copy link
Collaborator Author

I think the weights are just always normalized, right?

double histogramNormalizer{calcHistogramNormalizer(weights)};

anyways, I am not sure, whether the histogram itself is still normalized if weights are given...?

@Clownshift
Copy link
Collaborator Author

Side note, when debugging this, mind that currently, we do not check for negative weights (#58 )

@Clownshift
Copy link
Collaborator Author

I suggest to pull #60 first as soon as it is accepted.

@Clownshift
Copy link
Collaborator Author

Clownshift commented Oct 4, 2021

I started investigating a little bit and I noticed that the example notebook apparently still worked before #51.
@jokr91 Can you maybe check that out?

jokr91 added a commit that referenced this issue Oct 4, 2021
Due to the way std::accumulate decides which version is used (the initial value is the deciding factor), the sum was performed on integers, this leads to 0 values, when the parts of the sum are smaller 1. This should address and close #56 , and close #57.
Tests against this behavior should be added. Do not merge until I added these tests!
Clownshift pushed a commit that referenced this issue Oct 17, 2022
Due to the way std::accumulate decides which version is used (the initial value is the deciding factor), the sum was performed on integers, this leads to 0 values, when the parts of the sum are smaller 1. This should address and close #56 , and close #57.
Tests against this behavior should be added. Do not merge until I added these tests!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants