-
Notifications
You must be signed in to change notification settings - Fork 19
Added example notebook and changes for added user defined filter functions #88
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Added example notebook and changes for added user defined filter functions #88
Conversation
…eperate file and in a ipynb
|
Found 2 changed notebooks. Review the changes at https://app.gitnotebooks.com/sensorium-competition/experanto/pull/88 |
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.
Pull Request Overview
This PR adds support for user-defined filter functions via a registry and provides example implementations and usage in a notebook.
- Introduce
CALLABLE_REGISTRYandregister_callabledecorator for registering filter factories. - Extend
_get_callable_filterto instantiate filters configured with a__key__. - Add an example filter implementation in
common_filters.pyand a demo notebook showing how to plug in custom filters.
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| experanto/datasets.py | Added a callable registry and extended _get_callable_filter logic to handle __key__ mappings. |
| examples/my_functions/common_filters.py | Example custom nan_filter registered via register_callable. |
| examples/create_dataset_by_id.ipynb | Notebook demonstrating definition and use of user-defined filters. |
Comments suppressed due to low confidence (2)
examples/my_functions/common_filters.py:16
- [nitpick] The parameter name
device_is not very descriptive. Rename it to something likeinterpolatorordevice_interpolatorto clarify its purpose.
def implementation(device_: Interpolator, vicinity=vicinity):
experanto/datasets.py:360
- There are no unit tests covering the new registry-based instantiation path in
_get_callable_filter. Add tests for registering a callable, invoking it with valid arguments, and handling missing keys to ensure this feature remains reliable.
if isinstance(filter_config, (dict, DictConfig)) and "__key__" in filter_config:
| "from experanto.interpolators import Interpolator\n", | ||
| "\n", | ||
| "@register_callable(\"filter2\")\n", | ||
| "def id2interval(dataset=None, id_list=[], complement=False):\n", |
Copilot
AI
Jul 8, 2025
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.
Avoid using a mutable default argument id_list=[], which can lead to unexpected shared state between calls. Use id_list=None and initialize inside the function (e.g., if id_list is None: id_list = []).
| "def id2interval(dataset=None, id_list=[], complement=False):\n", | |
| "def id2interval(dataset=None, id_list=None, complement=False):\n", |
| return implementation_func | ||
| else: | ||
| raise ValueError( | ||
| f"Filter config string '{filter_config}' not found in CALLABLE_REGISTRY {CALLABLE_REGISTRY}." |
Copilot
AI
Jul 8, 2025
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.
The error message prints the entire filter_config dict, which may be verbose. Consider referencing only the missing key (filter_config['__key__']) to make the message clearer and more actionable.
| f"Filter config string '{filter_config}' not found in CALLABLE_REGISTRY {CALLABLE_REGISTRY}." | |
| f"Filter key '{filter_config['__key__']}' not found in CALLABLE_REGISTRY {CALLABLE_REGISTRY}." |
| self.interpolate_precision = interpolate_precision | ||
| self.scale_precision = 10**self.interpolate_precision |
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.
why are these parts deleted here?
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.
This is part of Rene's PR which I applied to my fork therefore it appeared in my PR #85 too
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.
This was simply moved to experiment from dataset since interpolation across devices has also been moved from dataset to experiment level via. get_data_from_chunks.
schewskone
left a comment
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
| def get_valid_range(self, device_name) -> tuple: | ||
| return tuple(self.devices[device_name].valid_interval) | ||
|
|
||
| def get_data_for_interval( |
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.
I could not find any use for this function did I miss it? Or is it also just part of Rene's PR?
|
we need to resolve #85 first and then adjust this one accordingly |
|
@goirik-chakrabarty am I missing sth or both of the example notebooks are actually unfinished? |
@pollytur Both the files look correct on my main branch apart from the usual user warnings. Am I missing something? Do I need to modify the PR to ensure these changes are reflected correctly? |
No description provided.