-
Notifications
You must be signed in to change notification settings - Fork 76
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
model fitting: handle empty poly order input #1041
Conversation
* traitlet now accepts the empty string, with form validation ensuring that it is never passed as such to python (can't click "add component" button while poly_order is empty) * if adopted, similar logic should be applied across all relevant plugin inputs
f850b6d
to
5e3f64b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1041 +/- ##
==========================================
+ Coverage 73.70% 74.03% +0.33%
==========================================
Files 74 75 +1
Lines 5620 5638 +18
==========================================
+ Hits 4142 4174 +32
+ Misses 1478 1464 -14 ☔ View full report in Codecov by Sentry. |
9ba7fbf
to
c8d56d7
Compare
* and add test coverage to catch any breaks from upstream changes
c8d56d7
to
81e69a7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works as advertised and the solution seems elegant. Thanks!
Though probably out of scope for the sprint but in the spirit of OSS, do you think traitlets
would accept your new classes if you try to submit a PR upstream? 💭
return super().validate(obj, value) | ||
|
||
|
||
class IntHandleEmpty(HandleEmptyMixin, Int): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this order done such that our validate would overwrite the validate from upstream? Just curious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, this uses the new validate
first (which then falls back on the super
validate on the upstream Int
/Float
traitlets if the value isn't an empty string).
Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I appreciate the addition of form validation guidance to the developer docs. I know I've tried to be good about using that in the past but haven't been totally consistent.
Description
This pull request removes the traceback caused by emptying the poly_order input in the polynomial component of the model fitting plugin.
If adopted, similar logic should be applied across all number input fields across all plugins in the future (created separate ticket).
Before:
Screen.Recording.2022-01-19.at.3.44.57.PM.mov
After:
Screen.Recording.2022-01-19.at.3.43.46.PM.mov
TODO:
Any
or subclassedInt
solutions. If usingInt
, determine where to place subclass and move it.Int
subclass doesn't break from upstream changesChecklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
trivial
label.CHANGES.rst
?