Add stage name #39

DavidKatz-il · 2020-10-08T19:45:07Z

marcglobality

Nice work! I added some comments and ideas, but I like a lot this addition

marcglobality · 2020-10-10T18:26:41Z

pdpipe/core.py

@@ -652,6 +655,17 @@ def __init__(self, stages, transformer_getter=None, **kwargs):
    def __getitem__(self, index):
        if isinstance(index, slice):
            return PdPipeline(self._stages[index])
+
+        if isinstance(index, list):


do you want to check for a list, or a list of strings?

I was not sure if it's right to force the label param to be an str.

marcglobality · 2020-10-10T18:28:29Z

pdpipe/core.py

+        if isinstance(index, str):
+            stages = [stage for stage in self._stages if stage._label == index]
+            if len(stages) == 0:
+                raise Exception("'{}' is not exist.".format(index))


if python3.5 support was dropped, this could be in f-strings, which is quite more readable @shaypal5

if two stages have the same label you will return just the first one in the pipeline. Maybe that's the desired behaviour, but highlighting just in case. Not sure whether we would like something when creating the pipeline to check for integrity (i.e. ensure that two stages cannot have same label), or we just trust the user

if python3.5 support was dropped, this could be in f-strings, which is quite more readable @shaypal5

I did not see the use of 'f-string' in the project, so I also did not use it.

if two stages have the same label you will return just the first one in the pipeline. Maybe that's the desired behaviour, but highlighting just in case. Not sure whether we would like something when creating the pipeline to check for integrity (i.e. ensure that two stages cannot have same label), or we just trust the user

When passing in one item a Stage is returned, so I cant return two stages.
pipeline[['someLabel']] will return a pipeline with all stages that label == 'someLabel'.

You're both right: I didn't f-strings so far, be we definitely should. I recently moved to using f-strings almost exclusively in my code. I'm also going into type hints, but I don't think it's a very urgent change.

Regarding the second issue: See discussion below. A reasonable handling of this case should be decided upon and documented.

re f-strings. We need to drop support for python3.5 or be very determined and use this https://stackoverflow.com/questions/55182209/f-string-invalid-syntax-in-python-3-5 , which I wouldn't recommend @shaypal5

Since Python 3.5 end-of-life was at September 2020, so Iet's do it!

I think it's better to make a separate PR of dropping python 3.5 and adding f-strings.

marcglobality · 2020-10-10T18:35:29Z

pdpipe/core.py

@@ -173,14 +173,16 @@ class PdPipelineStage(abc.ABC):
        dataframes, which will be used to determine whether this stage should
        be skipped for input dataframes. See pdp.cond for more information on
        specialised Condition objects.
+    label : str, default None
+        A label of this stage.


not sure I fully understand the label vs desc. Would we like the label to be the string representation then?

Nope. Label should be very short, like 'fix_spikes', while a description could be something like "Fixes spikes in all numeric columns by applying moving-window based smoothing function with parameters being dynamically learned, while smoothing, separately for each column."

You really couldn't use the later conveniently for access as you could the first, as in pipe['fix_spikes'] or for slicing, like pipe[['fix_spikes', 'drop_corr']].

marcglobality · 2020-10-10T18:38:49Z

pdpipe/core.py

@@ -191,6 +193,7 @@ def __init__(self, exraise=True, exmsg=None, desc=None, prec=None,
        self._prec_arg = prec
        self._skip = skip
        self._appmsg = '{}..'.format(desc)


should the label be used here if defined and no desc rather than DEF_DESCRIPTION?

I think the label should be added on the beginning of the application message. Something like

self._appmsg = '{}{}..'.format(f'{label}: ' if label else '', self.,desc)

... self._appmsg = f'{label}: {desc}')

No, stages with no supplied name shouldn't have an application message such as ": Dropping mostly NaN columns" (which is what the your current code does), but rather simply "Dropping mostly NaN columns".

Please fix to self._appmsg = '{}{}..'.format(f'{label}: ' if label else '', self.,desc)

marcglobality · 2020-10-10T18:41:43Z

pdpipe/core.py

@@ -191,6 +193,7 @@ def __init__(self, exraise=True, exmsg=None, desc=None, prec=None,
        self._prec_arg = prec
        self._skip = skip
        self._appmsg = '{}..'.format(desc)
+        self._label = label


I would check label and raise error if (at least) not a str

Agreed. Use with pytest.raises(SomeException) to check for correct error throwing behaviour.

def __init__(self, label='', ...) if not isinstance(label, str): raise Exception(f"label must be a str, not {type(label)}.") self._label = label ...

marcglobality · 2020-10-10T18:43:18Z

tests/core/test_pipeline.py

+    drop_char = SilentDropStage('char', label='dropChar')
+    pipeline = PdPipeline([drop_num1, drop_num2, drop_char])
+    assert len(pipeline) == 3
+    pipeline = pipeline[['dropNum1', 'dropNum2']]


the way it's implemented now, (I think) one can pass as labels integers, and slice here using integers which would make it a bit confusing I think. I would force the labels to be strings, and ensure people cannot pass a list of integers to slice. WDYT?

Notice that we are not going to disable int base slicing or access, as pipelines are first-and-formost lists of pipeline stages, and they should easily slice-able / index-accessible with integers. It's in the documentation.

DavidKatz-il · 2020-10-12T08:13:18Z

@shaypal5

Should we force label to be a string?
Should label be unique for all stages in a pipeline? if so where we need to check it because only when a stage is adding to the pipeline it has to be checked.

shaypal5 · 2020-10-12T09:55:05Z

I would say:

I think that we should. Looks reasonable enough. I don't see an inherent need for richer types of labels. This is already an enhancement, after all.
I'd say the package needs to handle such cases gracefully, not force it.

Either:

A pipeline checks - on creation - that all stage labels are unique, and if their not, issues a warning and disables label-based access and slicing
Do not ensure correct access and slicing when non-unique labels can be found in the same pipeline, and document it. When accessing/slicing, return any viable result for each of the non-unique labels asked for, and document that when that is the case there is no guarantee which pipeline stages will be returned, and that it is the user's responsibility to make sure each stage has a unique string label.

Also, notice that we are not going to disable int base slicing or access, as pipelines are first-and-formost lists of pipeline stages, and they should easily slice-able / index-accessible with integers. It's in the documentation.

DavidKatz-il · 2020-10-12T12:14:19Z

@shaypal5 WDYT about changing label to name similar to sklearn?

shaypal5 · 2020-10-14T08:47:05Z

Sure! A good change, I think, not to mix with dataframe column labels.

codecov-io · 2020-10-14T09:20:59Z

Codecov Report

Merging #39 into master will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master       #39   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           12        12           
  Lines         1719      1730   +11     
  Branches       232       238    +6     
=========================================
+ Hits          1719      1730   +11

Impacted Files	Coverage Δ
pdpipe/core.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cecda8d...7f21a24. Read the comment docs.

DavidKatz-il · 2020-10-14T09:27:40Z

I didn't use f-string because of the conflict with python 3.5.

shaypal5 · 2020-10-20T15:24:24Z

pdpipe/core.py

        self._exraise = exraise
        self._exmsg = exmsg
        self._desc = desc
        self._prec_arg = prec
        self._skip = skip
-        self._appmsg = '{}..'.format(desc)
+        self._appmsg = '{}: {}'.format(name, desc)


Please fix to
self._appmsg = '{}{}..'.format(f'{label}: ' if label else '', self.,desc)

Stages with no supplied name shouldn't have an application message such as ": Dropping mostly NaN columns", but rather simply "Dropping mostly NaN columns".

shaypal5 · 2020-10-20T15:32:16Z

Almost done!

See my latest comment on your app_desc, fix it and I'll pull this. :)

shaypal5 · 2020-10-30T10:49:45Z

Released in version v0.0.52 .

DavidKatz-il added 3 commits October 8, 2020 23:43

add parameter 'label' to PdPipelineStage

398317d

add options to slice a PdPipeline

2385890

add test for slicing a pipeline by labels

af61f2d

DavidKatz-il force-pushed the add-stage-label branch from 44edcd9 to af61f2d Compare October 8, 2020 20:47

marcglobality reviewed Oct 10, 2020

View reviewed changes

shaypal5 added enhancement good first issue labels Oct 12, 2020

label must be a str

e5cf0b1

DavidKatz-il added 2 commits October 14, 2020 12:19

change 'label' to 'name'

5c708ec

add documentation

9ff6c08

DavidKatz-il changed the title ~~Add stage label~~ Add stage name Oct 14, 2020

DavidKatz-il requested a review from shaypal5 October 14, 2020 09:33

shaypal5 reviewed Oct 20, 2020

View reviewed changes

update self._appmsg

7f21a24

shaypal5 merged commit cdce586 into pdpipe:master Oct 30, 2020

DavidKatz-il deleted the add-stage-label branch October 30, 2020 10:45

shaypal5 mentioned this pull request Oct 30, 2020

Feature suggestion: Add stage labels #19

Closed

Add stage name #39

Add stage name #39

Uh oh!

Conversation

DavidKatz-il commented Oct 8, 2020

Uh oh!

marcglobality left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcglobality Oct 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shaypal5 Oct 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavidKatz-il commented Oct 12, 2020

Uh oh!

shaypal5 commented Oct 12, 2020

Uh oh!

DavidKatz-il commented Oct 12, 2020

Uh oh!

shaypal5 commented Oct 14, 2020

Uh oh!

codecov-io commented Oct 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

DavidKatz-il commented Oct 14, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shaypal5 commented Oct 20, 2020

Uh oh!

shaypal5 commented Oct 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

marcglobality Oct 12, 2020 •

edited

Loading

shaypal5 Oct 12, 2020 •

edited

Loading

codecov-io commented Oct 14, 2020 •

edited

Loading

shaypal5 commented Oct 30, 2020 •

edited

Loading