Refactor #84
Refactor #84
Conversation
|
|
||
| let runBackwardsIncludeInPlace plus = runInPlace plus true scanInclusive | ||
|
|
||
| /// <summary> |
There was a problem hiding this comment.
А зачем удалять акие комментарии?
There was a problem hiding this comment.
Они перенесены в публичный api.
There was a problem hiding this comment.
Так а удалть зачем? Внутренняя документация тоже нужна.
| open GraphBLAS.FSharp.Objects.ArraysExtensions | ||
|
|
||
| module Radix = | ||
| module internal Radix = |
There was a problem hiding this comment.
А почему мы не хотим базовые функции сделать доступными широкой общественности?
There was a problem hiding this comment.
Из публичного api к ним можно получить доступ. Доступ к модулю с реализацией решил ограничить.
| /// </remarks> | ||
| /// <param name="clContext">OpenCL context.</param> | ||
| /// <param name="workGroupSize">Should be a power of 2 and greater than 1.</param> | ||
| let map2 (op: Expr<'a option -> 'b option -> 'c option>) (clContext: ClContext) workGroupSize = |
There was a problem hiding this comment.
А почему map остался, а map2 переехал?
There was a problem hiding this comment.
Да, нет причин не переносить. Сейчас перенесу.
|
А чего таки не собралось? |
There was a problem hiding this comment.
- Зачем прятать то, что может быть потенциально полезно? Я понимаю, зачем прятать функции, самостоятельный вызов которых может "всё сломать". Но безопасные зачем прятать?
- Комментарии. Не надо комментариев ради комментариев. И удалять комментарии, которые несут ценную информацию, тоже не надо
- Такое ощущение, что некоторые подпространства имён не такие и лишние. Типа сортрировок, которых много разных, BFS, может ещё чего.
|
|
||
| inherit WithoutTransferBenchmark<int>( | ||
| (singleSource ArithmeticOperations.intSumOption ArithmeticOperations.intMulOption), | ||
| (Algorithms.singleSourceBFS ArithmeticOperations.intSumOption ArithmeticOperations.intMulOption), |
There was a problem hiding this comment.
Вообще, bfs-ов может быть много разных. Так что может и стоит сделать подпространство Algorithms.BFS.
| /// ... | ||
| /// > val result = [| 3.6; 1.4; 3.6; 2.5 |] | ||
| /// </code> | ||
| /// </example> |
There was a problem hiding this comment.
Верните, пожалуйста, эти комментарии. Иначе как изучать саму реализацию?
| open GraphBLAS.FSharp.Objects.ClCellExtensions | ||
|
|
||
| module Reduce = | ||
| module internal Reduce = |
There was a problem hiding this comment.
Но зачем? Почему люди не могут пользоваться этим снаружи?
|
|
||
| ///<param name="clContext">.</param> | ||
| ///<param name="opAdd">.</param> | ||
| ///<param name="workGroupSize">Should be a power of 2 and greater than 1.</param> |
There was a problem hiding this comment.
Ну вот это же было полезное знание.
| | LIL of ClMatrix.LIL<'a> | ||
|
|
||
| /// <summary> | ||
| /// Row count. |
|
|
||
| let bitonic = | ||
| Sort.Bitonic.sortKeyValuesInplace clContext workGroupSize | ||
| Common.Bitonic.sortKeyValuesInplace clContext workGroupSize |
There was a problem hiding this comment.
А у нас же когда-то было две разных сортировки.
2 participants
Proposed Changes
Describe the big picture of your changes here to communicate to the maintainers why we should accept this pull request. If it fixes a bug or resolves a feature request, be sure to link to that issue.
Types of changes
What types of changes does your code introduce to GraphBLAS-sharp?
Put an
xin the boxes that applyChecklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...