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

Jaclyn taroni/2023 02 19 edits #242

Merged
merged 21 commits into from
Feb 21, 2023

Conversation

jaclyn-taroni
Copy link
Collaborator

Stacked on #238

Summarizing changes:

  • Changed Box 3 to follow one sentence per line convention. There are a couple minor changes in there as well.
  • Removed reference to complex phenotypes and complex data from the synopsis. I also changed our reference to "large amounts of complex data" because it was somewhat imprecise.
  • Fixed the Figure 1 legend per reviewer's comments.
  • Revised a bunch of language in Box 1 per reviewer's comments (e.g., "held-out test set" becomes "held-out validation set").
  • Added a TODO about updating the LASSO reference.

I'd like to take another look at Box 3 tomorrow, so I'm marking as a draft for now.

@jaybee84
Copy link
Owner

@jaclyn-taroni Thanks for starting this draft.

I was just taking a look at the changes, and it looks like a bunch of changes that I had already made in the original (word doc) did not port correctly to the PR .. e.g.

Removed reference to complex phenotypes and complex data from the synopsis. I also changed our reference to "large amounts of complex data" because it was somewhat imprecise.
Fixed the Figure 1 legend per reviewer's comments.
Revised a bunch of language in Box 1 per reviewer's comments (e.g., "held-out test set" becomes "held-out validation set").

Thanks for going through those and fixing/updating those. Looking forward to your finalized changes.

@jaclyn-taroni
Copy link
Collaborator Author

jaclyn-taroni commented Feb 21, 2023

I'm going to mark this ready for review for the benefit of running the other checks. Edit: That's not the issue, it's that I'm targeting a branch other than the default branch.

@jaclyn-taroni jaclyn-taroni marked this pull request as ready for review February 21, 2023 13:41
@jaclyn-taroni
Copy link
Collaborator Author

Okay, with c03e152, I made a few tweaks to where the new box (Box 1) is referenced. Some references seemed too low context given what the sentenced around them actually said – a reader is unlikely to be familiar with the papers cited – so I've either removed something or made it clear it was an example by adding e.g.,.

Copy link
Owner

@jaybee84 jaybee84 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added some minor comments. otherwise look good to me..

content/01.synopsis.md Outdated Show resolved Hide resolved
content/01.synopsis.md Outdated Show resolved Hide resolved
content/06.prior-knowledge.md Outdated Show resolved Hide resolved
content/05.model-complexity.md Outdated Show resolved Hide resolved
@jaybee84 jaybee84 mentioned this pull request Feb 21, 2023
6 tasks
@jaybee84
Copy link
Owner

@jaclyn-taroni I see that you did not make any changes to the outlook.md file where we refered to the excluded methods. Is it okay to assume that you did not have any additional edits there?

@jaclyn-taroni
Copy link
Collaborator Author

@jaclyn-taroni I see that you did not make any changes to the outlook.md file where we refered to the excluded methods. Is it okay to assume that you did not have any additional edits there?

Yes, no additional edits.

@jaybee84 jaybee84 merged commit c26f741 into jaybee84/edits021523 Feb 21, 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