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

FEAT: +TOKEN modifier for side-effect functions #15

Open
wants to merge 102 commits into
base: master
Choose a base branch
from

Conversation

ankostis
Copy link

Inputs & outputs in needs & provides kwargs designated as token work as usual while solving the DAG but they are never assigned any values to/from the operation's functions:

from graphkit import operation, compose
from graphkit.modifiers import token

# Function appending a new dataframe column from two pre-existing ones.
def addcolumns(df):
    df['sum'] = df['a'] + df['b']

# Designate `a`, `b` & `sum` column names as an token arguments.
graph = compose('mygraph')(
    operation(
        name='addcolumns',
        needs=['df', token('a'), token('b')],
        provides=[token('sum')])(addcolumns)
)

# The graph works with and without 'c' provided as input.
df = pd.DataFrame({'a': [5], 'b': [2]})
assert graph({'df': df})['sum'] == 11

I confirm that this contribution is made under the terms of the license found in the root directory of this repository's source tree and that I have the authority necessary to make this contribution on behalf of its copyright owner.

@yahoocla
Copy link

Thank you for submitting this pull request, however I do not see a valid CLA on file for you. Before we can merge this request please visit https://yahoocla.herokuapp.com/ and agree to the terms. Thanks! 😄

@ankostis
Copy link
Author

Just didi it.

Is it understood what I'm trying to achieve?

@codecov-io
Copy link

codecov-io commented Sep 28, 2019

Codecov Report

Merging #15 into master will increase coverage by 13.14%.
The diff coverage is 92.81%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #15       +/-   ##
===========================================
+ Coverage   77.87%   91.01%   +13.14%     
===========================================
  Files           5        6        +1     
  Lines         348      501      +153     
===========================================
+ Hits          271      456      +185     
+ Misses         77       45       -32
Impacted Files Coverage Δ
graphkit/modifiers.py 100% <100%> (ø) ⬆️
graphkit/functional.py 93.9% <100%> (+0.15%) ⬆️
graphkit/plot.py 89.68% <89.68%> (ø)
graphkit/base.py 81.01% <90.9%> (+1.64%) ⬆️
graphkit/network.py 94.14% <94.11%> (+23.94%) ⬆️

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 e70718b...3e06148. Read the comment docs.

Tokens work as usual while solving the DAG but
they are never assigned any values to/from the operation's functions.

+ TC included.
+ Docs updated.\+ Added `modifiers` superclass to facilitate identification code.
+ refactored FunctionalOperation._compute().
+ _norm_kwargs:
  + use isinstance() instead of type-equality checks,
    to support new modier classes;
    + avoid excessive dict searches with local vars.
not really needed, better be explicit which modifier is searched.
receiving partial inputs, needed for other operations.
+ The x2 TCs added just before are now passing.
NOTE dict are not deterministic in <PY3.6.
So this commit would not improve determinism
in those pythons.

+ build: add `boltons` dependency for ndexedSet.
+ doc: mark all set usage if affect determinism.
+ e.g. see reproducibility problem in yahoo#14.
needed to refactor the new pruning algorithm.

- expected 2 TCs fail for yet-to-be-solved yahoo#24 & yahoo#25 bugs.
override intermediate data.

More changes only for newer pruning TCs:
+ refact(test): rename graph-->netop vars for results of compose(),
  to avoid of `graph.net.graph`.
+ Explain failure modes in v1.2.4 & this merged branch (yahoo#19 + yahoo#23).
not after compose().

+ All TCs pass ok.
+ NOTE this is not yet what is described in yahoo#21.
... based on Law of Demeter simplify kwargs,
defined in one place.

+ enh: plotters "suggest" kwargs, possinly to override them
  (no duplcate arg when attempted).
+ FIX: apply thickness as in legend also for operators.
+ enh: don't cluster if no nodes pruned.
+ enh: netop includes its name a graph-title.
+ color palette: wheat filled nodes.
+ reuse common func.
... bugged in the opening commit d403783 of this PR, and
discovered 68(!) commits later, and all that time had to live
with x4 broken TCs with asked-outputs.
+ ENH: strongly refatctored TC to detect nulls & dupes.
+ Partial fix deterministic results (yahoo#22-2.4.3i).
+ test: some refacts on vars & for the uploaded images.
+ MARK SPURIOUS FAILS in < PY3.6 due to unordered dicts
  eg https://travis-ci.org/ankostis/graphkit/jobs/594813119
to support _repr_svg(), and it works without any flag.
+ graphop label renames,
+ DOT shortening.
+ FIX: `graph` is a DOT keyword, eg  pydot/pydot#111
  + Replaced `graph` --> `graphop`, inline with future refactoring.
+ Refactored example code.
+ New "Plotting" section under Quickstart.
+ Copy README instructions to main docs.
+ Abandoned `example_graph.svg`, replaced with `intro.svg`.
+ Recommend `.png` --> `.svg`, to save storage space.
- discovered BUG in MERGE saumple code (empty!).
... to adopt pytest args
+ mark wheel a universal.
ankostis referenced this pull request in syamajala/graphkit Oct 8, 2019
got values from v1.2.4
- ALL MERGE TCs FAIL!
many commits ago.

Never got it bc TC were not checking merges!
needed to give the sidefeect as input.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants