Clean parameterized hfun interface - #252
Conversation
Tox tasks were passing locally.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #252 +/- ##
==========================================
+ Coverage 94.49% 94.75% +0.25%
==========================================
Files 93 95 +2
Lines 3618 3831 +213
==========================================
+ Hits 3419 3630 +211
- Misses 199 201 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This one is really rough. I have to check for technical subtleties regarding this construction of anonymous functions. Test suite passed on GCE with 2025a.
I'm not sure what the bad class name complaint is about. Checking if name too long.
For Python. Identical to MATLAB version, except for the abs err tolerance for one special case test that is significantly larger in Python. All tests passing locally.
Synchronize `247NoKwargs` Branch
The changes were made in both the Python and MATLAB codes. The following
discussion is w.r.t. the Python code and hopefully helps explain the changes.
After designing the new test for this hfun, I suspected that the sign in front
of sumH in line 59 was incorrect. However, the tests were passing with small
errors. To determine if the test is capable of detecting that sign error, I
reran the tests but with the sign changed. There was effectively no difference
in the results despite the fact that the Hres and sumH variables contain
non-trivial numbers. This can be understood by seeing that
sumH * sum_{i=1}^m (Cres[i] - m_sumF) = sumH * 0.
Therefore, I removed the sumH term, which allowed for some simplifications,
especially in line 63.
Both Python and MATLAB tests were passing locally with no need to adjust error
tolerances in the test.
mmenickelly
left a comment
mmenickelly
left a comment
There was a problem hiding this comment.
Looks good to me and I appreciate the short write up about the changes to the square_diff_from_mean computations.
|
@jmlarson1 I made some additional changes to the code at 1ba39d3 since your review. Could you please review those before I merge? Hopefully the commit message will provide enough information for you to understand why those simplifications were made. |
|
@jared321 This is correct. In initial research, we played with different |
3 participants
PR Self-review
main