Add a stub overview section to folded report#356
Add a stub overview section to folded report#356JiriPavela merged 1 commit intoPerfexionists:develfrom
Conversation
tfiedor
left a comment
tfiedor
left a comment
There was a problem hiding this comment.
LGTM, made few suggestions that might help readability.
Also it might make sense to invest (you can try claude/codex/whatever) some effort to pack the whitespaces in the jinja template (in my experience, it can save quite some bytes in these reports).
perun/view_diff/report_folded.py
Outdated
| common_lf.bottom_k(records_num, by=exclusive_diff_key), | ||
| # Inclusive diff metric. | ||
| find_top_diffs(baseline_only_lf, records_num, symbol_key, inclusive_diff_key), | ||
| find_top_diffs(baseline_only_lf, records_num, symbol_key, inclusive_diff_key, False), |
There was a problem hiding this comment.
I would suggest to use keyword for the False so it is better readable, since this way one does not know immediately what is the parameter there), so top=False.
There was a problem hiding this comment.
Good idea, fixed.
perun/view_diff/report_folded.py
Outdated
|
|
||
|
|
||
| def find_top_diffs( | ||
| data_lf: pl.LazyFrame, count: int, symbol_key: str, diff_key: str, top: bool = True |
There was a problem hiding this comment.
I kindof like using * as follows:
| data_lf: pl.LazyFrame, count: int, symbol_key: str, diff_key: str, top: bool = True | |
| data_lf: pl.LazyFrame, count: int, symbol_key: str, diff_key: str, *, top: bool = True |
That forces the top to be used as keyword argument always. When you have many parameters or booleans I think it is godsend.
perun/view_diff/report_folded.py
Outdated
|
|
||
| :return: a lazy frame containing the maximal/minimal records. | ||
| """ | ||
| search_func = pl.LazyFrame.top_k if top else pl.LazyFrame.bottom_k |
There was a problem hiding this comment.
It would make sense to also switch from boolean parameter to supply the function; but looks like we have two uses cases anyway so it is fine.
There was a problem hiding this comment.
This would normally be the preferred option to a bool parameter, but the top_k and bottom_k polars functions have quite complex types, which would make the type hints of this little helper function too cumbersome.
The overview section will list top baseline-only, target-only, and common traces and functions w.r.t some diff metrics. This commit adds the propagation of the computed results into the HTML report.
JiriPavela
force-pushed
the
feature/report-overview-data
branch
from
43e0af8 to
21b6b94
Compare
This is a good suggestion. Rather than just removing the whitespace, I think we should instead try to use something similar to JS minification, which has the potential to save even more space. |
2 participants
The overview section will list top baseline-only, target-only, and common traces and functions w.r.t some diff metrics. This PR adds the propagation of the computed results into the HTML report so that the frontend can read the data.