Skip to content
This repository has been archived by the owner on Jun 22, 2022. It is now read-only.

Unintuitive adapter syntax #28

Closed
grzes314 opened this issue Apr 25, 2018 · 2 comments
Closed

Unintuitive adapter syntax #28

grzes314 opened this issue Apr 25, 2018 · 2 comments
Assignees

Comments

@grzes314
Copy link

Current syntax for adapters has some peculiarities.
Consider the following example.

        step = Step(
            name='ensembler',
            transformer=Dummy(),
            input_data=['input_1'],
            adapter={'X': [('input_1', 'features')]},
            cache_dirpath='.cache'
        )

This step basically extracts one element of the input. It seems redundant to write brackets and parentheses. Doing
adapter={'X': ('input_1', 'features')},
should be sufficient.

Moreover, to my suprise
adapter={'X': [('input_1', 'features'), ('input_2', 'extra_features')]},
is incorrect, and currently leads to
ValueError: too many values to unpack (expected 2)

My suggestions to make the syntax consistent are:

  1. adapter={'X': ('input_1', 'features')} should map X to extracted features.
  2. adapter={'X': [...]} should map X to a list of extracted objects (specified by elements of the list). In particular adapter={'X': [('input_1', 'features')]} should map X to a one-element list with extracted features.
  3. adapter={'X': ([...], func)} should extract appropriate objects and put them on the list, then func should be called on that list, and X should map to the result of that call.
@jakubczakon
Copy link
Collaborator

@grzes314 great idea, it seems intuitive to have it as you described.
Current api is (especially for just one element) very clunky. Also I think one option that could be useful is to be able to pass a chain of functions simply for example np.exp(lambda x: x[0]) or something.

@kamil-kaczmarek kamil-kaczmarek added this to the Steps as Python package milestone Apr 25, 2018
@grzes314 grzes314 self-assigned this Apr 25, 2018
kamil-kaczmarek pushed a commit that referenced this issue May 17, 2018
* Write tests for new adapter syntax

* Refactor adapter

* Improve handling of caches and logs in tests

* Fix minor issues mentioned in PR comments

* Rewrite tests in pytest framework

* Move adapting to seperate class, alter behaviour

* Correction: mutable object as default argument in Step initializer
@kamil-kaczmarek
Copy link
Member

merged in #42. Closing this.

kamil-kaczmarek pushed a commit that referenced this issue May 23, 2018
* initial

* cleaned up

* updates keras models and callbacks

* added explanation notebook

* renamed notebook

* ran notebook

* transformer loading now to be clicked

* dropped nlp

* fixed typos

* updated whats missing

* updated with dsb and talking data

* added simple pipeline notebook

* imports optimized

* feature_names=None and categorical_features=None

* refactor the way the model parameters are passed

* moved adapters to seperate file, renamed identity/take_first adapters

* Update misc.py

* fit_transform -> transform

* Update base.py

* fixed #10

* fixed typo

* * added: get_upsample_pad, get_downsample_pad
* added: multiclass_segmentation_loss

* *warning added

* Add some user interface messages

* Notebook#3: Adapter for ensembling

* Add example: 1-getting-started.ipynb

* Remove custom adapter; other minor changes after reviewer feedback

* Add example 2-multi-step.ipynb and some minor changes to example 1-getting-started.ipynb

* Cleanup of examples 1 and 2

* Fix a mistake in notebook text

* Add steps/__pycache__ to .gitignore

* Further changes based on Github feedback

* add self.conv_stride parameter in unet definition

* Change steps/__pycache__ to *.pyc in .gitignore

* Changes in Notebook#3 following code review

* train forests on train data and ensembler on ensembling data

* decoupled sklearn preprocessing from text preprocessing with heavy de… (#32)

* decoupled sklearn preprocessing from text preprocessing with heavy dependencies

* fixed import error

* dropped anonymization from text.py, reformatted imports, small refactors

* small refactor for readability

* Change time format (#40)

* Dev docstrings (#33)

* Step halfway done

* Step init args done

* Steps docstrings done

* added docstrings to Step properties

* Step docstring done

* dropped mock transformer addeed docstrings to Base and Dummy Transformers

* base documentation finished, minor refactors

* docstrings added to adapters

* docstrings added

* started refactoring lgbm

* Update base.py

* updated docstrings

* base transformer docstring fix

* fixed logging docstrings

* Notebook #5: Example with Keras (#41)

* Notebook #5: Example with Keras

* Corrected notebook for Keras and necessary refactors in ClassifierGenerator

* Fix issue #28: Unintuitive adapter syntax (#42)

* Write tests for new adapter syntax

* Refactor adapter

* Improve handling of caches and logs in tests

* Fix minor issues mentioned in PR comments

* Rewrite tests in pytest framework

* Move adapting to seperate class, alter behaviour

* Correction: mutable object as default argument in Step initializer

* Issue #16: make_transformer (#50)

* Corrections and tests for Sklearn wrappers

* make_transformer taking arbitrary functions

* fix issues #48 and #49 (#52)

* fix issues #48 and #49

* use of setdefault method

* fix according to Kamil's request

* Fix post merge issues

* Prepare package (#58)

* Remove tutorial notebooks

* Remove modules with heavy dependencies

* Rename steps -> steppy

* Add setup.py

* Tell git to ignore egg files

* Read the docs might output of sphinx-quickstart

* Requirements file

* Try adding sphinx-apidoc output

* Add some requirements

* Add IPython to requirements

* removed intro.ipynb and updated requirements.txt

* corrected setup.py

* refactored NoOperation to IdentityOperation, optimized inputs

* moved misc.py to steppy-toolkit

* simplified ProbabilityCalibation, BaseTransformer small refactor

* prepare setup.py and setup.cfg for PyPI registration

* added install requires to setup.py

* name refactor

* Create README.md
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants