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

Support per-sample weights #7

Closed
ATpoint opened this issue Jun 29, 2023 · 4 comments
Closed

Support per-sample weights #7

ATpoint opened this issue Jun 29, 2023 · 4 comments

Comments

@ATpoint
Copy link
Contributor

ATpoint commented Jun 29, 2023

Dear Rex,

I would like to follow up on (#3) towards support of per-sample weights. Did you happen to have any time supporting this for circacompare()? I tried naive solutions by providing a numeric vector of weights to nls() as an argument -- turns out it is more complex than that...

Do you currently have the capacity to help out here, or alternatively, can you point me to the sections in the source code that I need to dig myself into to address this?

Thank you very much!
-Alex

@RWParsons
Copy link
Owner

Hi Alex,

I forgot why I said it was hard before in the other issue but I think you'd need to add an argument to circacompare for the weights vector and then pass it to the call to stats::nls() here.

I'm busy for the next week or two but may have time to have another look at this after that. I'll keep it open but please give me an update on how you go over the next week or two before I have a go.

Thanks,
Rex

@ATpoint
Copy link
Contributor Author

ATpoint commented Jul 1, 2023

Hi Rex,

thank you for the response!

I think I managed now in https://github.com/ATpoint/circacompare/commit/0ceb788e0338db4dbe75e3cbbc6c22ffcb3db8c8

The tricky part was to realize that the nls() function creates its own local environment so one first has to add the weights to the x input data.frame which then allows to use the weights argument in nls, see here and here. If you provide the weights directly to nls() without adding to x first then you get an error as here at SO.

Limitation: I only added this to the nls() that runs the differential analysis, not to the model_each_group() function which assesses rhythmicity per group. For my specific use-case that's fine, so I will work with the forked version for now. Will close the issue for now. Happy to make a PR at some point if you find this generally reliable. Cheers!

Edit: Will make a PR with the points you mention below once I find the time, thanks!

@ATpoint ATpoint closed this as completed Jul 1, 2023
@RWParsons
Copy link
Owner

Thanks Alex,

Looks like this is a good fix that I'm sure would be helpful to others too. I'm happy to merge this in and add you as a contributor. Before you do a pull request, could you please add the sample_weights arg and functionality to the other circacompare functions (*_single and the *_mixed variants)?

@ATpoint
Copy link
Contributor Author

ATpoint commented Nov 6, 2023

All done in #8 :)

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

No branches or pull requests

2 participants