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

Added check function for weighted data #189

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

briederer
Copy link

I ran into a small problem, that curve_fit(model, xdata::AbstractArray, ydata::AbstractArray, wt::AbstractArray{T}, p0::AbstractArray; inplace = false, kwargs...) where T tried to fit my function for a weight 1/0 = Inf.
So I thought it would be useful to also check weights for NaN, Inf or Missing-values if provided.

@briederer
Copy link
Author

@pkofod Any chance to get this merged soon(ish)?
LsqFit.jl is currently breaking my full analysis-program and it would be nice to have this fixed 😅

If there are any objections let me know too so I can adapt it.

@pkofod
Copy link
Member

pkofod commented Oct 31, 2021

I'm very sorry for taking so long here. I think it would be a bit simpler if you just gave wt a default value that would succeed

check_data_health(xdata, ydata, wt=[])

or something similar

@briederer
Copy link
Author

Thanks for the comment.
Your suggestion seems to be the cleanest possibility.
I added the modification and also already rebased onto master.
Let me know if there is anything else I can do.

@briederer
Copy link
Author

Sorry for all the force-pushes but the commit history was kind of confusing and I wanted to clean it up.

@pkofod
Copy link
Member

pkofod commented Sep 2, 2022

I'm sorry for abandoning this discussion. Is this still relevant to you? If so, I would ask of you to add some tests, and then I will merge and release it asap.

@briederer
Copy link
Author

No worries 😉
So far I've simply used my fork.
IMHO I still think that also the weights should be checked for consistency unless a Inf weight has a special meaning which I've missed so far?
I'll add tests and rebase the PR as soon as I find some time.

@donovaly
Copy link

@briederer , maybe you have time to finish this PR?

@briederer
Copy link
Author

briederer commented Nov 15, 2024

Thanks for the bump.
Since I switched field of research in the mean time I did not need it anymore and completely forgot about this PR.
I'll put it on my todos.

@pkofod
Copy link
Member

pkofod commented Nov 16, 2024

@briederer I pushed to your branch. I think it's ready now, but tests don't deploy so I need to look into that

@briederer
Copy link
Author

Thanks @pkofod
I guess a maintainer needs to approve the workflow to start. (see here: https://docs.github.com/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks)

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.

3 participants