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.

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

Preprocessor refactor #1232

Merged
merged 15 commits into from Mar 29, 2017

Conversation

Projects
None yet
2 participants
@jlstevens
Copy link
Contributor

jlstevens commented Mar 29, 2017

This PR addressed #1224 by greatly simplifying how stream parameters can be renamed and how stream preprocessing is implemented (when necessary).

The rename suggestion from that issue has been implemented exactly as suggested but after discussion with @philippjfr and @jbednar, it was decided that we don't really need explicit preprocessor hooks. Instead, the streams that require it can implement the preprocess method (which occurs before stream renaming) so transform the stream values as necessary.

I would still like a better name than rename and although I personally like remap more, I know this isn't the majority opinion. Consider

PositionXY(rename={'x':'x1', 'y':'y1'}, x=2)

and

PositionXY(remap={'x':'x1', 'y':'y1'}, x=2)

I feel the latter conveys the idea that x will be called x1 on the output better. Any other suggestions?

Action items:

  • Remove old preprocessor system
  • Offer new simplified means of preprocessing streams
  • Implement rename constructor argument and method
  • Update PlotSize stream with a preprocess method
  • Add unit tests
  • Update tutorials Edit: postponed till a later PR

@jlstevens jlstevens added API WIP labels Mar 29, 2017

@jlstevens jlstevens added this to the v1.7.0 milestone Mar 29, 2017

@jlstevens

This comment has been minimized.

Copy link
Contributor Author

jlstevens commented Mar 29, 2017

After some discussion, we decided to rename the preprocess method to transform and have it set the transformed parameters in the update method. The PlotSize stream now implements scaling based on this approach.

In addition, stream parameters should now be set via the _set_stream_parameters helper method which handles the fact that stream parameters should always have constant=True.

@jlstevens

This comment has been minimized.

Copy link
Contributor Author

jlstevens commented Mar 29, 2017

I will update the tutorial material once I've finished going through DynamicMap. The tests should pass and this PR is ready for review.

@@ -115,7 +61,7 @@ def trigger(cls, streams):
stream.deactivate()


def __init__(self, preprocessors=[], source=None, subscribers=[],
def __init__(self, rename={}, source=None, subscribers=[],

This comment has been minimized.

@philippjfr

philippjfr Mar 29, 2017

Contributor

Did none of the docstrings mention preprocessors? Either way docstrings should now mention the rename argument.

This comment has been minimized.

@jlstevens

jlstevens Mar 29, 2017

Author Contributor

Fixed in 65131a2

'stream parameter of the same name' % v)
return mapping

def rename(self, **mapping):

This comment has been minimized.

@philippjfr

philippjfr Mar 29, 2017

Contributor

Add docstring.

This comment has been minimized.

@jlstevens

jlstevens Mar 29, 2017

Author Contributor

Fixed in 65131a2

@philippjfr

This comment has been minimized.

Copy link
Contributor

philippjfr commented Mar 29, 2017

Looks good, just update/add some docstrings.

@jlstevens

This comment has been minimized.

Copy link
Contributor Author

jlstevens commented Mar 29, 2017

Updated as suggested. Tests are passing for Python 2.7 and the Python 3 tests should pass soon,

@philippjfr philippjfr removed the WIP label Mar 29, 2017

@jlstevens

This comment has been minimized.

Copy link
Contributor Author

jlstevens commented Mar 29, 2017

The pr build just passed on the commit before I added the docstrings. I think it is ready to merge.

@philippjfr

This comment has been minimized.

Copy link
Contributor

philippjfr commented Mar 29, 2017

Merging.

@philippjfr philippjfr merged commit 25fde35 into master Mar 29, 2017

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+1.1%) to 78.076%
Details
s3-reference-data-cache Test data is cached.
Details

@philippjfr philippjfr deleted the preprocessor_refactor branch Apr 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.