Enhance mdexport to annotate rand() and randn() using sympy#63
Enhance mdexport to annotate rand() and randn() using sympy#63Utkarsha2811 wants to merge 3 commits into
Conversation
|
Hi @Utkarsha2811. Thanks for the PR! I won't be able to review it right away, so please be a bit patient. |
Thanks, Meanwhile I'll recheck and update PR description. |
|
Hi @Utkarsha2811, sorry for the delay in getting back to this PR. Since you opened it, the |
|
Hey @mstimberg, I've merged master so the test suite will run. I also took the opportunity to do some minor refactoring and added tests while I was at it. Let me know what you think! |
mstimberg
left a comment
mstimberg
left a comment
There was a problem hiding this comment.
Hi again @Utkarsha2811 and another apology for the long time it took me to review. This looks mostly good to me, but I have a few minor comments.
| # Use a negative lookahead so that randn() is not counted as rand(). | ||
| n_rand = len(re.findall(r'\brand(?!n)\s*\(', value_str)) |
There was a problem hiding this comment.
I don't quite understand why this needs a negative lookahead. A simpler \brand\s*\( wouldn't match randn() either, would it?
| rand_funcs = [f for f in funcs if f.func.__name__ == 'rand'] | ||
| randn_funcs = [f for f in funcs if f.func.__name__ == 'randn'] | ||
|
|
||
| if n_rand == 1 and n_randn == 0 and len(rand_funcs) == 1: |
There was a problem hiding this comment.
Isn't the check len(rand_funcs) == 1 redundant? What would be a situation where n_rand == 1 but len(rand_funcs) != 1?
| max_tex = sympy.latex(max_val, mode='plain') | ||
| annot_tex = fr"{var_tex} \in [{min_tex}, {max_tex}]" | ||
|
|
||
| elif n_randn == 1 and n_rand == 0 and len(randn_funcs) == 1: |
There was a problem hiding this comment.
Same question as for line 714
| var_val = sympy.simplify(coeff ** 2) | ||
|
|
||
| mean_tex = sympy.latex(mean_val, mode='plain') | ||
| var_tex = sympy.latex(var_val, mode='plain') | ||
| annot_tex = fr"\mu={mean_tex},\ \sigma^2={var_tex}" |
There was a problem hiding this comment.
Both are of course possible, but I think I'd prefer having the std-dev instead of the variance, in particular because it has more natural units.
2 participants
Problem: The Markdown exporter currently displays raw strings for initializations using rand() and randn() without providing mathematical context.
Solution: I used SymPy to analyze these expressions. The exporter now calculates and displays exact lower/upper bounds for rand() and the mean/variance for randn() in the exported Markdown descriptions.