REFACTOR-#3257: Move logging and caching to the gen_data function #7046
Conversation
arunjose696
requested review from
devin-petersohn,
mvashishtha,
RehanSD,
YarShev,
vnlitvinov,
anmyachev,
dchigarev and
a team
as code owners
… function Signed-off-by: arunjose696 <arunjose696@gmail.com>
arunjose696
force-pushed
the
gen_data_log_refactor
branch
from
4da597f
to
f51b046
Compare
YarShev
changed the title
|
@anmyachev, any comments? |
| nrows, ncols, rand_low, rand_high | ||
| ) | ||
| ) | ||
| data = gen_int_data(nrows, ncols, rand_low, rand_high).copy() |
There was a problem hiding this comment.
Is gen_int_data used anywhere else? If not, we could remove it.
There was a problem hiding this comment.
This function would be called from gen_data function as data_generator can be gen_int_data.
asv_bench/benchmarks/utils/common.py
Outdated
| assert data_type in type_to_generator | ||
| data_generator = type_to_generator[data_type] | ||
|
|
||
| data = data_generator(nrows, ncols, rand_low, rand_high) | ||
| data = data_generator(nrows, ncols, rand_low, rand_high, cache_key) |
There was a problem hiding this comment.
and remove that new line from the lower level functions?
| data = data_generator(nrows, ncols, rand_low, rand_high, cache_key) | |
| data = data_generator(nrows, ncols, rand_low, rand_high, cache_key) | |
| data_cache[cache_key] = weakdict(data) |
asv_bench/benchmarks/utils/common.py
Outdated
| ) | ||
| ) | ||
| data = gen_int_data(nrows, ncols, rand_low, rand_high).copy() | ||
| data = gen_data("int", nrows, ncols, rand_low, rand_high).copy() |
There was a problem hiding this comment.
Why call a higher level function from a lower level function? gen_int_data will be called anyway, right?
There was a problem hiding this comment.
If we call gen_int_data the data wouldnt be cached as the caching logic has been moved to get_data, Thus I figured calling gen_data higher level function would be better to cache data for later.
There was a problem hiding this comment.
If we call gen_int_data the data wouldnt be cached as the caching logic has been moved to get_data,
That's ok, we shouldn't use a low level function if there is a way to use it through a high level function.
There was a problem hiding this comment.
So I dont need to make any changes here right? (As gen_data higher level function is called)
There was a problem hiding this comment.
In fact, we should leave this line as it was before, imagine the execution flow, we design it so that it starts with function gen_data, since it contains additional caching functionality. If the function of the lower part of the flow (for example, gen_str_int_data) calls the upper one, then the flow is not unidirectional (this is usually more difficult to work with and understand).
…ired Signed-off-by: arunjose696 <arunjose696@gmail.com>
arunjose696
force-pushed
the
gen_data_log_refactor
branch
from
c0d2868
to
5016902
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
arunjose696
force-pushed
the
gen_data_log_refactor
branch
2 times, most recently
from
4d03c40
to
80c0147
Compare
|
asv job failed with |
Co-authored-by: Anatoly Myachev <anatoliimyachev@mail.com> Signed-off-by: arunjose696 <arunjose696@gmail.com>
arunjose696
force-pushed
the
gen_data_log_refactor
branch
from
80c0147
to
f53c7bf
Compare
What do these changes do?
flake8 modin/ asv_bench/benchmarks scripts/doc_checker.pyblack --check modin/ asv_bench/benchmarks scripts/doc_checker.pygit commit -sgen_datafunction #3257docs/development/architecture.rstis up-to-date