Skip to content

BUG: Effect size simulation#100

Closed
daikitag wants to merge 1 commit intotskit-dev:mainfrom
daikitag:effect-size-sim
Closed

BUG: Effect size simulation#100
daikitag wants to merge 1 commit intotskit-dev:mainfrom
daikitag:effect-size-sim

Conversation

@daikitag
Copy link
Copy Markdown
Collaborator

The original code was dividing the effect sizes by the number of causal sites, which led to the simulation of traits having extremely small variance.

I modified the codes for all trait models in tstrait, such that the mean and variance of the overall phenotype are balanced.

I would really like to thank Alison for spotting this by reading the tstrait manual.

@jeromekelleher
Copy link
Copy Markdown
Member

I think we need to do a few more things @daikitag as this is a serious bug:

  1. Create an issue to track the bug that we can refer to (not this PR). This should describe the bug clearly and precisely, and document any effects on downstream users
  2. Update the manual
  3. Update the changelog to briefly describe the bug, and provide a link to corresponding issue and PR(s)

@daikitag
Copy link
Copy Markdown
Collaborator Author

I think we need to do a few more things @daikitag as this is a serious bug:

  1. Create an issue to track the bug that we can refer to (not this PR). This should describe the bug clearly and precisely, and document any effects on downstream users
  2. Update the manual
  3. Update the changelog to briefly describe the bug, and provide a link to corresponding issue and PR(s)

Thanks Jerome. I will dramatically update the manual for effect size simulation in the next PR.

@codecov
Copy link
Copy Markdown

codecov Bot commented Nov 15, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (5371a97) 100.00% compared to head (f175d07) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #100   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            8         8           
  Lines          302       302           
  Branches        31        31           
=========================================
  Hits           302       302           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

The original code was dividing the effect sizes by the number of causal sites, which led to the simulation of traits having extremely small variance. All the trait models are modified to account for that.
@jeromekelleher
Copy link
Copy Markdown
Member

LGTM, but I didn't realise this was a problem in the first place and it's not obvious to me that we've solved it.

I think the deeper problem here is that we are not comparing our results against other programs. We should be doing this - I'll open an issue to track.

@jeromekelleher
Copy link
Copy Markdown
Member

Thanks Jerome. I will dramatically update the manual for effect size simulation in the next PR.

Can you open an issue to track that then please? We want a set of modular issues that we can address before we release the update .

@jeromekelleher jeromekelleher added this to the 0.0.2 release milestone Nov 15, 2023
@daikitag
Copy link
Copy Markdown
Collaborator Author

LGTM, but I didn't realise this was a problem in the first place and it's not obvious to me that we've solved it.

I think the deeper problem here is that we are not comparing our results against other programs. We should be doing this - I'll open an issue to track.

I agree. I will examine how other simulators are simulating effect sizes, and I will let you know after I figure it out.

@daikitag
Copy link
Copy Markdown
Collaborator Author

Removing this pull request, due to changes made in #107. This architecture will be implemented following the feedback in #101.

@daikitag daikitag closed this Nov 17, 2023
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.

2 participants