[WIP] FIx smoothing and aggregation for unbalanced expected#466
[WIP] FIx smoothing and aggregation for unbalanced expected#466
Conversation
|
I am confused about the test, if I manually run the same command that it should be running it works for me, while pytest complains about exit code 2. |
agalitsyna
left a comment
agalitsyna
left a comment
There was a problem hiding this comment.
I don't fully agree with aesthetics of the changes, maybe reconsider introduction of cols dictionary.
| "n_pixels": _NUM_VALID, | ||
| "n_contacts": "count.sum", | ||
| "contact_freq": "count.avg", | ||
| "smooth_suffix": ".smoothed", |
There was a problem hiding this comment.
Consider renaming "suffix", "smooth suffix seems rather confusing and hard to generalize in future.
| # additional smoothing and aggregating options would add columns only, not replace them | ||
| if smooth: | ||
| if clr_weight_name is None: | ||
| # result["count.avg"] = result["count.sum"] / result["n_valid"] |
There was a problem hiding this comment.
What is this commented out line?
| assert _delta_smooth_agg < 0.02 | ||
|
|
||
|
|
||
| def test_expected_smooth_nobalance_cli(request, tmpdir): |
| # add smoothed columns to the result (only balanced for now) | ||
| result = result.merge( | ||
| result_smooth[["balanced.avg.smoothed", _DIST]], | ||
| result_smooth[[cols["contact_freq"] + cols["smooth_suffix"], _DIST]], |
There was a problem hiding this comment.
I find this notation very hard to read. Is it possible to retain something like this?
"count.sum" + suffix / without dictionary, having the regular notation
I endorse both readability and flexibility of the code, and here it looks like the balance is shifted towards flexibility.
| cols["contact_freq"] | ||
| + cols["smooth_suffix"]: cols["contact_freq"] | ||
| + cols["smooth_suffix"] | ||
| + ".agg" |
There was a problem hiding this comment.
".agg" is not in the cols dict anymore, although it's another type of suffix!
Consistency would be great.
3 participants
No description provided.