petrelharp / ftprime_ms

4 stars 2 forks source link

Krt edits #39

Closed molpopgen closed 6 years ago

molpopgen commented 6 years ago

Edits from the holiday read-through. I have yet to go over the appendix in detail.

molpopgen commented 6 years ago

Yes, that is true, but I think it will be true fairly generally. Simplifying every generation will reduce the speedups by a fair bit, based on my experiments.

On Wed, Dec 27, 2017 at 2:22 PM Peter Ralph notifications@github.com wrote:

@petrelharp commented on this pull request.

In forwards_paper.tex https://github.com/petrelharp/ftprime_ms/pull/39#discussion_r158874871:

\begin{figure} \includegraphics[]{sims/speedup} \caption{\label{fig:relative_speedup_selection}Relative performance improvement due to pedigree tracking. This figure shows the ratio of the run time tracking neutral mutations over the run time tracking the pedigree. Data points are taken from Figure~\ref{fig:runtimes_selection} for simulations that ran to completion using both methods.} \end{figure}

+The peak RAM use in gigabytes (GB) is shown for the simulations with selection in Figure~\ref{sfig:ramsel} aand for the +simulations without selection in Figure~\ref{sfig:ramnosel}. The general picture is that pedigree tracking requires +more RAM. With \fwdpy{}/\fwdpp{}, the reason for this behavior is clear. When pedigree tracking, a non-recombinant

Or, rather: this seems like a useful point to make, but perhaps "pedigree tracking requires more RAM" should say instead "As we implemented these simulations, pedigree tracking required more RAM." ... ?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/petrelharp/ftprime_ms/pull/39#discussion_r158874871, or mute the thread https://github.com/notifications/unsubscribe-auth/AGHnH02aSSJ1Nxj8IbOuk1OvzD35hFfBks5tEsJngaJpZM4RN1Pv .

--

Kevin Thornton

Associate Professor

Ecology and Evolutionary Biology

UC Irvine

http://www.molpopgen.org

http://github.com/ThorntonLab

http://github.com/molpopgen

petrelharp commented 6 years ago

Nice; I like it. I put in comments. What do you think about a Methods section?

molpopgen commented 6 years ago

Nice; I like it. I put in comments. What do you think about a Methods section?

I'll address the comments early tomorrow, and also read through the algorithm descriptions more carefully.

As for the methods--hard to say. It is probably a good idea to write down our version numbers, etc., to "future-proof" the manuscript a bit. That's methods-y. Algorithmic stuff is probably fine as-is. Curious what the others thing?

ashander commented 6 years ago

Nice. I will read through now, adding comments if I see anything.

As for the methods--hard to say. It is probably a good idea to write down our version numbers, etc., to "future-proof" the manuscript a bit. That's methods-y. Algorithmic stuff is probably fine as-is. Curious what the others thing?

I agree that for submission we should have at least the version numbers our code requires. I will add more thoughts after reading through

ashander commented 6 years ago

On the methods: I'd love to consider our code + docs as details on our implementation and provide version numbers/commit hashes/etc to facilitate reproducibility.

I think @petrelharp was suggesting more details, and I can see the argument that reviewers may want that. Maybe we can head that off by adding more detailed summary of our specific implementations as 'Methods"?

After this is merged, with our without my suggestion above, I"m happy to take the editing reigns for a couple days. I can add in a short summary of this type and make other edits too

molpopgen commented 6 years ago

I've addressed some of @ashander's comments. I think this one can be merged. I want to go through my GC interval testing results. There may be something there worth presenting. If the GC interval is too small, it looks like run times could double or triple, but I should quantify this. Doing so would be a separate PR, so that someone else can hack up the manuscript in the mean time.

ashander commented 6 years ago

👍

On Thu, Dec 28, 2017 at 13:06 Kevin R. Thornton notifications@github.com wrote:

I've addressed some of @ashander https://github.com/ashander's comments. I think this one can be merged. I want to go through my GC interval testing results. There may be something there worth presenting. If the GC interval is too small, it looks like run times could double or triple, but I should quantify this. Doing so would be a separate PR, so that someone else can hack up the manuscript in the mean time.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/petrelharp/ftprime_ms/pull/39#issuecomment-354357745, or mute the thread https://github.com/notifications/unsubscribe-auth/AAfLOOuIQ5KRxsyTAGtrN_SudU0PCD7Wks5tFAKAgaJpZM4RN1Pv .

-- -Jaime

petrelharp commented 6 years ago

re: more details in the methods - I am agnostic, I think. Version numbers it seems can be provided in the code, alternatively.

petrelharp commented 6 years ago

and, edit token to @ashander . thanks!