-
Notifications
You must be signed in to change notification settings - Fork 86
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Redesign Adapter subsystem #876
Conversation
Hello @gkirgizov! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2022-09-13 14:29:43 UTC |
21a0c8c
to
32d95ee
Compare
@pep8speaks pls update youself |
32d95ee
to
a63e414
Compare
Codecov Report
@@ Coverage Diff @@
## master #876 +/- ##
==========================================
- Coverage 88.01% 87.98% -0.03%
==========================================
Files 198 201 +3
Lines 13479 13528 +49
==========================================
+ Hits 11863 11903 +40
- Misses 1616 1625 +9
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
9f5f286
to
25a1ad7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Предлагаю сразу на основе описания PR-а сделать заметку в документации, посвященную именно адаптерам. Можно со ссылками на докстринги (т.к. они довольно подробные), которые нужно почитать.
25a1ad7
to
8f5fe7b
Compare
a1e27f4
to
67980a7
Compare
…en adapting population
ece6a1c
to
b63e0f1
Compare
raise NotImplementedError() | ||
|
||
@abstractmethod | ||
def _restore(self, opt_graph: OptGraph, metadata: Optional[Dict[str, Any]] = None) -> DomainStructureType: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Может тогда этот метод назвать restore_graph или restore_single?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Аналогично с _adapt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, не заметил твой коммент перед мерджем.
вообще, это непубличный метод, и его не предполагается вызывать извне. это скорее ядро реализации метода, для определения в потомках.
Adaptation subsystem is refactored -- now it's more high-level and declarative, requiring less attention from developers.
Main changes:
It works like this:
adapt_func( f(Pipeline)->Pipeline ) => f'(OptGraph)->OptGraph
That is, from the function that can work with domain graphs (e.g. Pipeline) we get a function that can work with OptGraphs (it automatically adapts its arguments and return type).
isinstance(mutation_type, Callable)
) is dropped -- it is determined automatically inside BaseAdapter.adapt_population
@register_native
-- these functions are supposed to work withOptGraph
directly.See doc-comment of the new class
AdaptRegistry
for more detailed description.Aimed at #742