No longer allow Parameter as input to ModelComponents#198
Conversation
henrikjacobsenfys
added
[scope] maintenance
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #198 +/- ##
===========================================
- Coverage 98.13% 98.10% -0.04%
===========================================
Files 51 51
Lines 3542 3532 -10
Branches 652 646 -6
===========================================
- Hits 3476 3465 -11
Misses 36 36
- Partials 30 31 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
henrikjacobsenfys
changed the title
damskii9992
left a comment
damskii9992
left a comment
There was a problem hiding this comment.
You have some more cleanup to do :) Your code gets much cleaner now that you don't allow Parameters as inputs ;)
Otherwise, LGTM.
| if not isinstance(area, Numeric): | ||
| raise TypeError('area must be a number.') | ||
|
|
||
| if isinstance(area, Numeric): |
There was a problem hiding this comment.
You can remove this if isinstance as you already made that check above.
There was a problem hiding this comment.
Pretty sure this check is redundant now, since the min/max is by default -inf and inf.
| raise ValueError('area must be a finite number or a Parameter') | ||
|
|
||
| raise ValueError('area must be a finite number.') | ||
| area = Parameter(name=name + ' area', value=float(area), unit=unit) |
There was a problem hiding this comment.
Just realized you're overwriting the local area variable. It is better coding practice to name it something else to avoid accidental coding errors. Like area_param or something.
There was a problem hiding this comment.
You should also be able to remove this check and the relevant argument, I suppose.
| if not isinstance(width, Numeric): | ||
| raise TypeError(f'{param_name} must be a number.') | ||
|
|
||
| if isinstance(width, Numeric): |
There was a problem hiding this comment.
You should also be able to remove this else branch
2 participants
Closes #62