[ENH] Initial type and named object validator code#122
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
RNKuhns
changed the title
| require_unique_names=False, | ||
| sequence_name: Optional[str] = None, | ||
| ) -> Union[Sequence[Tuple[str, BaseObject]], Dict[str, BaseObject]]: | ||
| ... # pragma: no cover |
Check notice
Code scanning / CodeQL
Statement has no effect
| require_unique_names=False, | ||
| sequence_name: Optional[str] = None, | ||
| ) -> Sequence[Tuple[str, BaseObject]]: | ||
| ... # pragma: no cover |
Check notice
Code scanning / CodeQL
Statement has no effect
| require_unique_names=False, | ||
| sequence_name: Optional[str] = None, | ||
| ) -> Union[Sequence[Tuple[str, BaseObject]], Dict[str, BaseObject]]: | ||
| ... # pragma: no cover |
Check notice
Code scanning / CodeQL
Statement has no effect
fkiraly
left a comment
fkiraly
left a comment
There was a problem hiding this comment.
This looks very useful!
One comment, based on a learning in sktime and a guess on what this will be for:
I recently changed the checking logic to allow lists of tuples of arbitrary length, where element 0 is name/str, element 1 is estimator, and the remaining elements can be anything (e.g., index specifiers in column estimators etc).
This is because there were a number of examples where heterogeneous estimators had more than two elements, and it turned out to be easiest to relax the typing constraint.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #122 +/- ##
==========================================
+ Coverage 78.07% 81.92% +3.84%
==========================================
Files 23 30 +7
Lines 1884 2235 +351
==========================================
+ Hits 1471 1831 +360
+ Misses 413 404 -9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| def check_sequence( | ||
| input_seq: Sequence[Any], | ||
| sequence_type: Optional[Union[type, Tuple[type, ...]]] = None, | ||
| element_type: Optional[Union[type, Tuple[type, ...]]] = None, | ||
| coerce_output_type_to: type = None, | ||
| coerce_scalar_input: bool = False, | ||
| sequence_name: str = None, | ||
| ) -> Sequence[Any]: |
Check notice
Code scanning / CodeQL
Returning tuples with varying lengths
|
@fkiraly I've got updates and additional testing taken care of. I'm thinking we initially restrict the named object format to the current (str, BaseObject) or Dict[str, BaseObject], then relax it in a follow-on PR. The reason I want to take a bit more time to get familiar with different use cases of named objects and consider if there are any specific patterns we want to check. For example, (str, BaseObject, str). And whether we want to allow certain dictionary formats for these options (maybe Dict[str, (BaseObject, str)] but that is just off the top of my head). |
4 participants
Reference Issues/PRs
What does this implement/fix? Explain your changes.
Does your contribution introduce a new dependency? If yes, which one?
What should a reviewer concentrate their feedback on?
Any other comments?
PR checklist
For all contributions
the PR topic is related to enhancement, CI/CD, maintenance, documentation, or a bug.
For code contributions