Synthesis: CRT, signed mul, log-depth sum, sub and mul - #119
Conversation
2a62559 to
e49ef53
Compare
e49ef53 to
e33d71e
Compare
spefk
left a comment
spefk
left a comment
There was a problem hiding this comment.
Пушу часть комментов, так как подробно ревьюить все методы я буду еще 10 лет. Предлагаю все таки распилить этот ПР на несколько -- в каждом отдельный тип арифметических генераторов -- только так, по кусочкам, сможем эту всю радость отревьюить и вмержить.
| if label in mem: | ||
| return mem[label] | ||
| gate = gates[label] | ||
| if gate.gate_type.name == "INPUT": |
There was a problem hiding this comment.
Нужно сравнивать не имя, а сам тип "gate.INPUT" (где gate это импортированный модуль), например как тут
cirbo/cirbo/core/circuit/circuit.py
Line 676 in 4af4413
| gates = self.gates | ||
| mem = {} | ||
|
|
||
| def depth(label): |
There was a problem hiding this comment.
В целом, Ок, но при глубине > ~1000 может падать по RecursionError из-за естественного ограничения на количество рекурсивных вызовов. В идеале стоит переписать на процедурный стиль (через очередь).
|
|
||
| :return: integer value representing the maximum depth of the circuit | ||
| """ | ||
| gates = self.gates |
There was a problem hiding this comment.
Минорное имхо: так стоит делать только тогда, когда получение объекта действительно сложное (например self.gates.values.first.gate.gate_type и так далее). А сейчас gates, во первых, перекрывает импорт модуля gates из-за коллизии имени, а во вторых, мешает понимать что gates это не локальная переменная а часть состояния объекта.
There was a problem hiding this comment.
- Наименование модуля (файла) должно быть строчными буквами
- Не заметил, он не экспортируется никуда? А используется где-то?
| "more-itertools >= 10.8.0", | ||
| "typing-extensions >= 4.14.1", | ||
| "python-sat >= 1.8.dev26", | ||
| "python-sat (==1.8.dev30)", |
There was a problem hiding this comment.
А почему не "python-sat 1.9.dev7"? Давай изменения зависимостей (и .lock файл, и апдейт pybind11 и mockturtle соответственно) отдельным ПРом лучше протащим быстренько, чтобы в рамках полученного коммита было видно на что это повлияло (это в целом хорошая практика -- отделять обновления зависимостей/окружения с соответствующими фиксами кода для совместимости от основных изменений в логике)
Я обновил python-sat в отдельном ПРе
There was a problem hiding this comment.
Про все новые методы: нужно проверить coverage новых методов. Тесты должны покрывать все основные ветки выполнение (включая разные положения флагов типа big_endian).
| ZERO = xor_two_bits(circuit, x[0], x[0], basis=basis) | ||
|
|
||
| _basis = GenerationBasis(basis.upper()) if isinstance(basis, str) else basis | ||
| if _basis == GenerationBasis.XAIG: |
There was a problem hiding this comment.
Здесь, и везде где проверяется значение enum'а, нужно явно прописывать elif с нужным значением enum'a, а в else падать с ошибкой вида "unsupported enum value". Это нужно для того, чтобы при добавлении нового значения enum'a не пришлось бояться за неконсистентность остального кода (самый страшный баг -- который молча портит данные).
Это в целом касается чистоты программирования, не только про enum'ы а про все подобные конструкции.
| input_labels: tp.Iterable[gate.Label], | ||
| *, | ||
| big_endian: bool = False, | ||
| basis: tp.Union[str, GenerationBasis] = GenerationBasis.XAIG, |
There was a problem hiding this comment.
А зачем разрешать тут str? Может, не надо?
| """ | ||
| Computes the logical depth of the circuit. | ||
|
|
||
| The depth of a circuit is defined as the length of the longest path from any |
There was a problem hiding this comment.
Нам разве важно "до любого другого"? Нам тут важно до любого выходного гейта ведь -- до него и нужно замерять?
There was a problem hiding this comment.
В third_party залетели старые версии -- нужно обновить сабмодули и убрать эти изменения из ПРа
2 participants
This PR extends the capabilities of the generators with the following improvements: