feat: add AVLSet implementation, unit tests and benchmarks#17
Conversation
gsvgit
left a comment
There was a problem hiding this comment.
Лучше явно разделить дерево и множество.
|
|
||
| let value n = | ||
| match n with | ||
| | Empty -> failwith "Empty node has no value" |
There was a problem hiding this comment.
Посмотрите внимательнее. Мы стараемся не использовать failwith. Пользуемся result вместо этого.
There was a problem hiding this comment.
А имеет ли смысл Result, если скрыть от пользователя модули Tree и Node? Для пользователя есть только модуль AVLSet, который гарантирует, что эти ошибки в принципе недостижимы (по аксиомам АВЛ-дерева) при вызове функций, иначе ошибка в логике. Все-таки у меня задача написать множество на основе дерева, а не само дерево.
| balance ln rnNew v | ||
| | _ -> | ||
| match right with | ||
| | Empty -> failwith "Unreacheable message 2" |
There was a problem hiding this comment.
Не очень информативное сообщение.
There was a problem hiding this comment.
Изменил их все на более информативные, но также оставил предложение, что эти сообщения в принципе не должны быть достижимы.
| let mutable leftUnion = Empty | ||
| let mutable rightUnion = Empty | ||
|
|
||
| Parallel.Invoke( |
There was a problem hiding this comment.
Уверены, что через таски - лучший вариант?
Ну и в целом, посмотрите, как пространства имён в .net устроены. Например, на array.parallel
There was a problem hiding this comment.
Переделал все на async. Разделил пространства
|
Да. Внятное описание к реквесту тоже не будет лишним. |
|
gsvgit
left a comment
There was a problem hiding this comment.
У Вас конфликты. И не надо липить сопоставление с образцом везде.
|
|
||
| let value n = | ||
| match n with | ||
| | Empty -> failwith "Empty node has no value. This error should be unreachable" |
There was a problem hiding this comment.
Посмотрите на другой код в библиотеке. Мы не используем Failwith (стараемся, по крайней мере). Вместо этого --- Result
| match n with | ||
| | Empty -> Node(0, value, Empty, Empty) | ||
| | Node(h, v, ln, rn) -> | ||
| match value with |
|
|
||
| module ParallelAVLSet = | ||
| let rec unionAsync threads set1 set2 = | ||
| async { |
There was a problem hiding this comment.
А async точно быстрее? Почему его выбрали?
There was a problem hiding this comment.
Нет, он не быстрее тасков, если говорить о работе с деревьями. Но он подходит больше для ФП стиля (без mutable), чем таски. Плюс у Async холодный старт. Поэтому и изменил
There was a problem hiding this comment.
Видимо, вопрос в том, зачем именно Вы делали параллельную версию.
There was a problem hiding this comment.
Сравнить производмтельность операций с разными реализациями. Параллельные гораздо быстрее
There was a problem hiding this comment.
Было бы здорово добавить описание бенчмарок (примерно, как тут сделано). Ну и ссылку в основное ребми тоже. А в Вашем случае ещё и результаты замеров хотя бы в описании реквеста сделать.
|
|
Насчет Result: Я не совсем понимаю необходимость в нем. Стандартные библиотеки в F# также выбрасывают исключение в случае передачи невалидных аргументов. А если я перепишу все на Result, то у меня буквально весь код и все операции превратятся в нагромождение Ok и Error, даже функция add будет это возвращать, хотя это не очень удбно для конвейера |
Необходимость в том, что это прототип и будет переписываться на языки, где исключений нет. А чтобы не было чуть проще жить, в основной ветке уже даже есть workflow соответствующий. Можете изучить, ребейзнуться и пользоваться. |
- Encapsulate implementation by marking Node and Tree as internal. - Keep AVLSet module public as the primary API. - Implement InternalsVisibleTo to allow testing of internal structures. - Move parallel operations to a dedicated sub-module for better SoC. - Add FsCheck property-based tests for core set operations. - Fix and stabilize benchmarks.
…n matching, open modules, clean up redundant pattern matchings and replace them with if-else
|
Удалил все исключения и заменил все на Result<AVLSet<'A>,AVLSetError>. Ну, и, соответственно, бенчмарки с тестами тоже |
No description provided.