Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Support for multiple input data objects in Syncopy decorators #47

Merged
merged 4 commits into from
Apr 27, 2020
Merged

Support for multiple input data objects in Syncopy decorators #47

merged 4 commits into from
Apr 27, 2020

Conversation

pantaray
Copy link
Member

@pantaray pantaray commented Apr 24, 2020 •
edited

Author Guidelines

  • Is the change set < 400 lines?
  • Was the code checked for memory leaks/performance bottlenecks?
  • Is the code running locally and on the ESI cluster? new queuing system prevented me from starting a SLURMCluster in the 'DEV' queue - only tested w/LocalCluster for now fixed w/FIX: Repair SLURMClusters with Python execs in user homes #51
  • Is the code running on all supported platforms? untested on Windows

Reviewer Checklist

  • Are testing routines present?
  • Do parallel loops have a set length and correct termination conditions?
  • Do objects in the global package namespace perform proper parsing of their input?
  • Do code-blocks provide novel functionality, i.e., no re-factoring using builtin/external packages possible?
  • Code layout
    • Is the code PEP8 compliant?
    • Does the code adhere to agreed-upon naming conventions?
    • Are keywords clearly named and easy to understand?
    • No commented-out code?
    • Is a file header present/properly updated?
  • Are all docstrings complete and accurate?

@pantaray
CHG/NEW: Rewrite of wrapper_cfg to handle multiple input objects
307e5c4 
- the `unwrap_cfg` decorator has been extended to properly
  process not only single input data objects but arbitrary
  many (akin to MATLAB's `varargin`). Now, any function `func`
  decorated with `unwrap_cfg` can support (if wanted) things
  like
  ``func(data1, data2, data3, a="a", b=2)``
  or equivalently
  ``dList = [data1, data2, data3]``
  ``func(*dList, a="a", b=2)``
  or equivalently using a `cfg` "structure"
  ``cfg.data = [data1, data2, data3]; cfg.a = "a"; cfg.b = 2``
  ``func(cfg)``
- the "inner" decorator `unwrap_select` has been modified as well
  to expect `data` being a list, not a single Syncopy data object.
  If in-place selection is used w/multiple input objects, each
  object must be compatible w/the provided selection.
- with an invasive modification like this, a dedicated test-suite
  seemed warranted, thus a new test file test_decorators.py has
  been added. Even for single-object tests, this might be a good
  idea anyway, to ease debugging w/complex algorithms (to faster
  determine if an error is rooted in a decorator or the algorithm)
- todo: update docstrings in kwargs_decorators.py and fix a minor
  bug where a wrong `cfg` type triggers an `IndexError`, instead of
  the intended `SPYTypeError`.

Changes to be committed:
	modified:   syncopy/examples/ex_specest.py
	modified:   syncopy/shared/kwarg_decorators.py
	new file:   syncopy/tests/test_decorators.py
@pantaray
CHG/NEW: Finished tests and improved object propagation
17a1a09 
- fixed bug in `unwrap_cfg` that prevented `SPYTypeError`
  to be raised for non-dict-like `cfg` inputs
- fixed a bug in `unwrap_cfg` that caused input objects provided
  as multiple positional args (`data1`, `data2`, ... or `*datalist`)
  to be propagated in reversed order (careless use of `pop()` instead
  of `pop(0)`).
- changed logic of `unwrap_cfg`: do not propagate data objects encapsulated
  in a list but instead unfold the constructed `data` list by calling the
  decorated function using
  ``func(*data, *posargs, **kwargs)``
  This way, `unwrap_cfg` does *not* change the type of `data` input objects
  (from Syncopy data to list).
- changed `unwrap_select` following modifications of `unwrap_cfg`:
  cycle through anonymous `args` and attach `select` dictionary (if
  provided) to any object in `args` that has a `_selection` attribute.
- changed `detect_parallel_client` following modifications of `unwrap_cfg`:
  use standard Python call signature `func(*args, **kwargs)` to invoke the
  wrapped function
- updated docstrings of all decorators to reflect introduced changes

Changes to be committed:
	modified:   syncopy/shared/kwarg_decorators.py
	modified:   syncopy/tests/test_decorators.py
@pantaray pantaray added the Feature Request A concrete request (as detailed as possible) to either add or change functionality. label Apr 24, 2020
@pantaray pantaray self-assigned this Apr 24, 2020
@pantaray pantaray linked an issue Apr 24, 2020 that may be closed by this pull request
Extend unwrap_cfg to work w/multiple input data objects #46
Closed
joschaschmiedt
joschaschmiedt approved these changes Apr 27, 2020
View reviewed changes
Copy link
Collaborator

@joschaschmiedt joschaschmiedt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the the unwrap_cfg decorator and am impressed by it's versatility (very clever how the docstring is manipulated).

If I understand the code correctly the wrapper also supports multiple positional data arguments, i.e.

result = func(cfg, data1, data2, data3)

I did not see any documentation or tests for this use case. Is that right?

@pantaray
Copy link
Member Author

If I understand the code correctly the wrapper also supports multiple positional data arguments, i.e.

result = func(cfg, data1, data2, data3)

I did not see any documentation or tests for this use case. Is that right?

Yes, the documentation does not explicitly mention this use-case at the moment. This should be included in our developer docs (I'll open an issue addressing this). Regarding tests: the new testing module test_decorators.py contains a function test_varargin which (among other things) tests this specific call style (line 201: fnameList = group_objects(cfg, *self.dataObjs))

@pantaray pantaray mentioned this pull request Apr 27, 2020
Closed
@pantaray
CHG: Replaced str-based type checking w/cleaner isinstance approach
41fc80a 
- instead of using `str(type(obj))` to check for Syncopy objects
  use the cleaner `isinstance`-approach instead (following the
  discussion in PR #47)

Changes to be committed:
	modified:   syncopy/shared/kwarg_decorators.py
@pantaray
Merge branch 'dev' into unwrap_cfg_extension
6475274
@pantaray pantaray merged commit 25fff08 into dev Apr 27, 2020
@pantaray pantaray deleted the unwrap_cfg_extension branch April 27, 2020 14:54
@pantaray pantaray restored the unwrap_cfg_extension branch April 27, 2020 15:00
@pantaray pantaray deleted the unwrap_cfg_extension branch August 10, 2021 09:21
@pantaray pantaray restored the unwrap_cfg_extension branch August 10, 2021 15:15
@tensionhead tensionhead deleted the unwrap_cfg_extension branch August 11, 2021 13:33
@pantaray pantaray restored the unwrap_cfg_extension branch March 3, 2022 10:56
@tensionhead tensionhead deleted the unwrap_cfg_extension branch March 3, 2022 13:59
@pantaray pantaray restored the unwrap_cfg_extension branch March 3, 2022 14:00
@pantaray pantaray deleted the unwrap_cfg_extension branch March 9, 2022 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joschaschmiedt joschaschmiedt joschaschmiedt approved these changes

Assignees

@pantaray pantaray

Labels
Feature Request A concrete request (as detailed as possible) to either add or change functionality.
Projects
No open projects
Alpha 2: Summer 2020
  
Done
Milestone
Alpha 2: Wavelet, Coherence and MATLAB
Development

Successfully merging this pull request may close these issues.

Extend unwrap_cfg to work w/multiple input data objects
2 participants
@pantaray @joschaschmiedt