SciML linting - allow nn outputs not in params table - #511
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #511 +/- ##
==========================================
+ Coverage 75.61% 75.80% +0.18%
==========================================
Files 65 65
Lines 7354 7357 +3
Branches 1323 1323
==========================================
+ Hits 5561 5577 +16
+ Misses 1289 1280 -9
+ Partials 504 500 -4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
dweindl
left a comment
There was a problem hiding this comment.
Thanks, good after gating on SciML-usage and not hiding the import error.
| try: | ||
| from .extensions.sciml_lint import get_nn_entity_petab_ids | ||
| except ImportError: | ||
| pass |
There was a problem hiding this comment.
This pass doesn't feel safe. I think get_nn_entity_petab_ids should only be imported/called if problem required SciML, but then, any import failure should be an error.
There was a problem hiding this comment.
Makes sense, I've updated accordingly.
dilpath
left a comment
There was a problem hiding this comment.
Looks good but I think the wording is a bit suboptimal. NN outputs should never appear in the parameter table, right? But currently the PR seems to imply that there are cases where it is OK
I thought this was the case, but wasn't 100% sure. I have updated the wording now to say they should not appear. |
Co-authored-by: Dilan Pathirana <59329744+dilpath@users.noreply.github.com>
Allow network outputs to be used in observable and noise formulae without having to appear in the parameters table. The PR updates the
get_required_parameters_for_parameter_tablecheck to reflect this.