Feature #252: clearer error and check for emissivity function#254
Feature #252: clearer error and check for emissivity function#254
Conversation
|
Hello @lasofivec! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2019-11-19 18:02:52 UTC |
|
Good idea,
|
Didou09
left a comment
Didou09
left a comment
There was a problem hiding this comment.
All godd, but just taking over to polish (PEP + if instead of asserts)
| ###################################################################### | ||
| # Signal calculation | ||
| ###################################################################### | ||
| cdef get_insp(ff): |
There was a problem hiding this comment.
Ok, so I understand you are taking the function checking out of _GG to place into _core right ?
That's fine for me, just want to be sure I get it ;-)
| and pp.default is not pp.empty)] | ||
| return na, kw | ||
|
|
||
| def check_ff(self, ff, t=None, ani=None): |
There was a problem hiding this comment.
All good to me, the more detailed the error messages, the better it is for everyone indeed :-)
| time_steps = -1 | ||
| # .. Checking basic definition of function .......................... | ||
| str_error = "Input emissivity function (ff): " | ||
| assert hasattr(ff, '__call__'), (str_error |
There was a problem hiding this comment.
However, small caveat: the sue of assert, though compact, is not the best option in my sense.
If statements would be bit better because:
- more readable code structure
- The error messages are defined only if the check is not passed (with assert, the message has to be defined even if there is no error).
- This technical reason
I'll take over from here just to turn asserts into if, for good habits
There was a problem hiding this comment.
ok, the code was originally yours :)
Codecov Report
@@ Coverage Diff @@
## devel #254 +/- ##
==========================================
+ Coverage 43.85% 43.91% +0.05%
==========================================
Files 74 74
Lines 21462 21518 +56
==========================================
+ Hits 9412 9449 +37
- Misses 12050 12069 +19
Continue to review full report at Codecov.
|
| try: | ||
| out = ff(test_pts, t=t) | ||
| except Exception: | ||
| assert False, (str_error |
There was a problem hiding this comment.
To force raise an exception just use
raise Exception(msg)
(assert False if overkill)
| + " ff(Pts, t=t), where Pts is a (3, npts) np.array and" | ||
| + " t a len()=nt iterable," | ||
| + " must return a (nt, npts) np.ndarray!") | ||
| assert (type(out) is np.ndarray |
There was a problem hiding this comment.
Good practice : try using isinstance(var, class) instead of type(var) is class whenever possible
(I used to do the same, sorry)
| + " np.ndarray when Pts is (3, npts), vect is" | ||
| + " provided and t is None or a scalar") | ||
| assert type(out) is np.ndarray and out.shape == (1, npts), msg | ||
| return |
There was a problem hiding this comment.
This function should return is_ani
There was a problem hiding this comment.
Because:
- It used to in the original version
- the default value of ani is None, but the functions called later take ani as input and expect it to be True or False. This function function (check_ff) determines whether ani is True or False, even when it is None. It checks and format ani for other functions called later
…queeze if necessary (but only if necessary)
4 participants
Cleaner errors when user didn't define a correct emissivity function.
For example, if using function in #248 (comment), we get:
Solves #252