Refactor/refactor AlphaPeel#292
Conversation
|
Coverage report of the functional test: |
gregorgorjanc
left a comment
gregorgorjanc
left a comment
There was a problem hiding this comment.
The changes look good/ok
|
Functional tests memory profiling - this is not very informative, as the Numba function can be called at any part of the run: |
Coverage report cannot identify the calls to jit-compiled functions. |
|
The original accuracy tests have been split into two parts:
Still under development... |
|
GitHub runner does not support Python 3.7 for ubuntu: for simplicity, just test version |
I am trying to see if rearranging some code in There are a few Numba options that might be interesting, and I want to give them a try. But after that, I want to focus on the formality of the code that completes
|
|
I tried both Attempts were also made for the parallelisation of |
|
Parallelising the |
|
The recent commit 74349a3 updates
|
|
The plan for the rest of this PR is:
|
gregorgorjanc
left a comment
gregorgorjanc
left a comment
There was a problem hiding this comment.
I guess it would make sense to then also use snake_case in all file and folder names.
|
There is a |
|
The test failed because my local file system cannot recognise the change of file name from |
|
|
Real Data Evaluation ReportEvaluated the change so far against two different chromosomes of real data. In summary, memory usage dropped significantly, and runtime decreased as well. The number of page faults decreased. MemoryThe memory usage decreased by roughly 1/4.
RuntimeThe runtime usage also decreased, although not as big a portion as tested with small-sized data, maybe because the optimisation was mainly focused on the setup and writeout instead of the computation.
AccuracyAccuracy is almost the same before and after the change.
Page faultsThe number of page faults (invalid memory access) is decreased after the change, possibly due to better variable access practice.
ConclusionOverall, we improved the code efficiency and quality without unexpected impact on the accuracy of the outputs. |
|
Excellent evaluation @XingerTang! I agree that these diffs look good. Now that you have run on the larger data - do you see any scope for optimisation of calculations instead of mostly IO? |
I was trying different things today. By noticing that the arrays generally have the last dimension being the dimension of
|
|
I think we have a realistic marker density for SNP array genotype data in the example compared to what is being used in breeding programmes (can you remind me of the number?), but these densities are likely to grow once low-coverage sequencing will become more commonly used. I guess this means that we need different marker densities in our example - one with current density of about O(10^3) markers per chromosome and another with O(10^4 to 10^6). Ideally we would just add O(10^6) if time would not be too much for development (if it is, then pick a lower number). This is one of the reasons Andrew proposed hybrid option, where the second peeling pass is fully parallelised - we can talk more about this in person. |
|
I just tested and confirmed that Eddie doesn't require The The reason why the loci-wise parallelisation implemented earlier was not working is probably that the speedup is so small. On the local laptop, the speedup was only 1/60, which can easily be overtaken by the overhead. |
In the real data, the number of loci is 2951. If the number of loci is larger, then parallelisation over loci would be more effective. The single-loci peeling can indeed be fully parallelised over loci. I'm always wondering about the aim of hybrid peeling, as it wasn't the fastest nor is it more accurate. This makes a lot of sense. |
Parallelisation schemesI changed the scheme for parallelisation over loci; previously, the loci were sliced every time each function was called. Now, the loci are sliced at the creation of the peeling_info object, and each thread only operates on its sliced component. While the slices are merged together for the segregation probabilities calculation and final output, this way saves the cost of individual slicings during the computation and improves the speed. The code is quite messy now, so only the runtime report is presented here. Experiment setupThe runtime presented here is measured only with the simulation data that has 1000 loci per individual, 200 individuals per generation, 5 generations in total, and 100 parents (50 families) per generation. The option The ResultsLocal laptopThe runtime against the number of threads using different parallelisation methods on a local laptop is illustrated below: 
For both methods, the runtime decreased first with increasing number of threads, but later the overhead exceeded the speed improvements, and the runtime is longer with more threads. The EddieOn the same dataset, the same experiment on Eddie gives: The overall trends on Eddie are the same as the local laptop: the runtime first decreases and later increases as the number of threads increases. There are two main differences:
The fastest run also happened when SummaryThe newer scheme is useful in improving the runtime efficiency, and the improvement is more substantial on Eddie. The best combination for the given simulation setup is |
|
Good testing @XingerTang. While you see improvements, they are not large, though ~20% is not bad. Does this mean that there is not much left to be optimised with the current data structures? I have some ideas about the later - we will talk next time in person. |
I agree that it would be quite difficult to optimise further without making some sacrifice given the current data structure. The only thing I can think of is #293, which enables the reuse of the calculations of penetrance probabilities of missing loci instead of repetitive calculations on vectors of [0.25, 0.25, 0.25, 0.25]. |
2 participants
Related Issue
Closes #78
Closes #147
Closes #172
Closes #178
Closes #258
Working...
What changed
ValueErrorinstead of printing error messagesWorking...
Bigger picture of the full plan
Modification on tests:
Refactor the reassignment of argument options in AlphaPeel to remove warnings raised because of the unsynced arguments retrieval.
Remove unused/commented code
Replace single-letter variable names with more informative names
Add comments and docstrings when applicable
Memory optimisation
Move the functions that are jit-compilable to jit
Explore multi-processing solution on HPC
Why this change
Working...
Submodule changes
Notes / Risks
Working...