Run optimize #275

tech4GT · 2018-06-01T17:24:12Z

fixes #269 @jywarren
I have leveraged the progressObj, since inside of node we only call the sequencer run once and we need the progress bar also
but in demo we don't need progressBar and we need run(index) so here index is passed in place of progressObj(We can rename this parameter to suit this new functionality)
Now this is perfectly optimized and only the current step is run, not the whole sequence

PS I Know this is based on an old fork, I'll rebase once you approve of this

Signed-off-by: tech4GT <[email protected]>

jywarren

Looks like a good start! I want to ensure this code is really readable and I'm also not sure about overloading the function unless we have very good documentation -- because the variable names in the function can be a little confusing then. Can you explain the different parameters that are now possible? Thanks!!

jywarren · 2018-06-01T18:10:59Z

src/ImageSequencer.js

    }
-
+    console.log(index)


Don't forget to remove this

jywarren · 2018-06-01T18:12:40Z

src/ImageSequencer.js

    if(arguments[0] != 'test'){
      progressObj = spinnerObj
-      delete arguments['0']    
+      delete arguments['0']
+      if(typeof progressObj == 'number') {


Hmm, this is a bit hard to read - are you detecting if it's an index integer? If so can you explain a bit more in comments so it's more readable? Thanks!

jywarren · 2018-06-01T18:14:04Z

examples/lib/defaultHtmlSequencerUi.js

@@ -47,7 +47,7 @@ function DefaultHtmlSequencerUi(_sequencer, options) {
    var newStepName = $(addStepSel + " select").val();
    _sequencer
      .addSteps(newStepName, options)
-      .run(null);
+      .run(_sequencer.images.image1.steps.length - 2);


Hmm, how does this work? Can you leave a comment explaining?

Signed-off-by: tech4GT <[email protected]>

tech4GT · 2018-06-02T07:02:59Z

@jywarren I have added explanatory comments everywhere and changed the parameter name!!

tech4GT · 2018-06-02T07:04:26Z

@jywarren It's not at all complicated, for a module writer nothing changes!! For us though, in Node.js first argument is the progressObject and in demo the first argument is the index from which we wish to run the sequencer!!

tech4GT · 2018-06-02T07:05:30Z

this first argument was being ignored in demo until now since we don't need the cli progress object in demo, so we are just using a part of the api that already existed but was not being used!!

jywarren · 2018-06-02T15:34:50Z

But if we add a new module in Node.js, does it rerun from the beginning each time? I.e. does this optimization affect both node and browser modes? Thanks!

…

On Sat, Jun 2, 2018, 1:06 AM Varun Gupta ***@***.***> wrote: this first argument was being ignored in demo until now since we don't need the cli progress object in demo, so we are just using a part of the api that already existed but was not being used!! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#275 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ9Ag1hLFmb0H7U0yZJEDS1SaQ79qks5t4jk6gaJpZM4UXDG6> .

jywarren · 2018-06-02T18:25:27Z

Think about this in terms of how you'd explain it to a newcomer looking to use this library. Would you say:

sequencer.run() -- this method accepts an optional parameter run(index) in the browser, but a different run(progressObject) parameter in Node.js

This is a little weird if we are telling people that the library works the same in different environments. What are our options?

maybe use an options hash, so we'd submit .run({index: 1}) or .run({progressObject: ____}) so you never get mixed up?
could we build in this functionality to the addStep/insertStep functions so it runs identically in both node and browser?

I think run() is a bit confusingly documented in general. I think we should think about how to really clearly present how it's supposed to be used between:

https://github.com/publiclab/image-sequencer#running-the-sequencer

and

https://github.com/publiclab/image-sequencer#running-a-sequencer-with-multiple-images

What do you think about all this? Do you have some ideas for how the run() function can be more clearly and completely explained?

Thanks Varun! This is great.

tech4GT · 2018-06-02T18:30:37Z

@jywarren Actually sequencer already does not rerun in node since sequence is predefined

jywarren · 2018-06-02T18:33:08Z

Maybe we start by listing out the different variants of run() usage. What are the current vs. new variants?

run(callback)
run(image, index, callback)
run(json) with { img_1: index, img_2: index }
we also had (though not documented): - run(spinnerObj, t_image, t_from) -- isn't that right?

New:

run(spinnerObj, t_image, t_from) ...

What else?

jywarren · 2018-06-02T18:33:11Z

examples/lib/defaultHtmlSequencerUi.js

@@ -29,7 +29,7 @@ function DefaultHtmlSequencerUi(_sequencer, options) {

  function removeStepUi() {
    var index = $(removeStepSel).index(this) + 1;
-    sequencer.removeSteps(index).run();
+    sequencer.removeSteps(index).run(sequencer.images.image1.steps.length-1);


Here's where maybe we should think about whether run should auto-detect that nothing has changed in certain steps? Or do you think it makes sense to do this in the UI only?

@jywarren I think this only makes sense in the UI !!

Is that because when we are in Node, we aren't immediately running run() when we add steps? I almost think that would be a scenario where we'd want the sequencer to know that nothing has changed on prior steps, automatically, and not have to re-run the math for prior steps.

Like each step would potentially have a .changed property, and doing .insertStep() would switch that to true for any subsequent step. What do you think of this "switch-based" idea?

@jywarren I don't fully understand sorry, in node we run one predefined sequence right? So how would we be able to call insertStep in node?? I mean we can't modify the sequence during execution in node right?

Well, i think we can set this aside for now based on my other comments. But just imagine --

var sequencer = require("../image-sequencer")(); ... sequencer.run(); sequencer.insertSteps('blur', 2); sequencer.run(); // here, we wouldn't have to re-run steps 1 or 2, right?

@jywarren Oh I got it!!! I was confusing node.js as CLI!! Obviously we need this functionality in node yeah!! let's do the thing where we pass in the values as keys in an object like {index: ,progressObj: }

OK -- that sounds good. We can still do nice thorough docs as mentioned earlier, but just focusing on the different parameters rather than different types. 👍

tech4GT · 2018-06-02T18:34:43Z

@jywarren Here is my view of run function:
sequencer.run(optionalParam) where optionalParam car represent an offset if it is a number or the progressObject if it is an object. We can do option 1 i guess but I think this is not that complicated
Simply sequencer.run() can be called in 2 ways sequencer.run(index) or sequencer.run(progressObj) everywhere, it's just that we won't be needing one of these in one of the environments

tech4GT · 2018-06-02T18:36:54Z

@jywarren all this just adds 2 more usages to the existing list we already have documented, gives us the ability to pass either an index or a progressObject as the first argument

tech4GT · 2018-06-02T18:37:39Z

or maybe we can have these as keys inside an object as you suggested earlier in option 1, what do you suggest??

tech4GT · 2018-06-02T19:20:56Z

@jywarren According to me we can handle the current api with comments and documentation

jywarren · 2018-06-02T20:02:32Z

Can you suggest some documentation text for the progressObj usage so we can look at it all together?

…

On Sat, Jun 2, 2018, 1:21 PM Varun Gupta ***@***.***> wrote: @jywarren <https://github.com/jywarren> According to me we can handle the current api with comments and documentation — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#275 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ192qrTJ3F0QSAMmKKRshoveuFgzks5t4uWYgaJpZM4UXDG6> .

tech4GT · 2018-06-02T20:03:48Z

@jywarren I'll write some now!!

jywarren · 2018-06-02T20:06:32Z

Awesome, thanks Varun.

…

On Sat, Jun 2, 2018, 2:04 PM Varun Gupta ***@***.***> wrote: @jywarren <https://github.com/jywarren> I'll write some now!! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#275 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ3aBN9mQP35suCFL6PVIefMJ8kChks5t4u-kgaJpZM4UXDG6> .

Signed-off-by: tech4GT <[email protected]>

tech4GT · 2018-06-02T20:18:24Z

@jywarren Have a look please!!

tech4GT · 2018-06-03T01:26:41Z

So lets merge the other pr and rebase and then I’ll do this to avoid merge conflicts

jywarren

OK, I've been thinking about this. I agree with what you're saying that we can handle the current api with comments and documentation. I just needed to think through it a bit!

So, I really like your new documentation -- just a few comments/suggestions on it; take a look!

Thanks for thinking through this carefully with me! .run() is a central function to the library and it's super important that it be well thought through and stable.

jywarren · 2018-06-03T14:45:49Z

README.md

@@ -204,13 +204,32 @@ modules.
 sequencer.run();
 ```

+Sequencer can be run with a custom progress object


Just above here, where we introduce .run(), let's say:

## sequencer.run() The `.run()` method starts the sequencer, running through each step to process the input image into a final output image. There are several different ways to use the `run()` function. You can pass in: * `index` -- an `integer` - * `progressObj` -- an `object` used for a progress-tracking object that will be passed along the sequence (see Progress Tracking, below) * `callback` -- a `function` to be run upon completion of the sequence They can be used in different combinations: `sequencer.run(index)` - run the sequencer from the given `index` starting step position `sequencer.run(progressObj)` - ...and so on, showing each usage separately (so not mixing `index/progressObj` in one example) For examples of how `.run()` can be used, see [this test file demonstrating each use](...).

What do you think of this? Just really thorough, lots of examples, and link to the tests where we demonstrate it all working? The tests could be moved into a run.js test file.

@jywarren this is awesome, I can do this, this is really explanatory!! Let's first complete the urlSteps PR then rebase to avoid merge conflicts later since this is on an old fork!!

tech4GT · 2018-06-03T16:13:45Z

@jywarren So what I am doing on this now is refactoring run to accept the first parameter as an object which contains both index and progressObj, let's call it config and these are both optional and sequencer can be called without the config as well!!

Signed-off-by: tech4GT <[email protected]>

tech4GT · 2018-06-07T17:24:40Z

@jywarren I think we are done with this one!! Great!! Now should we complete our step api issue first or move to meta modules??

jywarren · 2018-06-07T17:38:15Z

test/modules/run.js

+test('run() works with all possible argument combinations',function(t){
+    sequencer.run();
+    sequencer.run({index: 0});
+    sequencer.run({mode:test},function callback(out){


looks like test should be in quotes? Or do you pass in the test object?

Also, do you want to somehow try to confirm which steps are actually run? Maybe you could start by running only the last one, and assert that the earlier steps have no output. Then run() and see that all have outputs?

oh, yeah I'll do that!!

tech4GT · 2018-06-07T18:40:11Z

@jywarren I am facing trouble finding which steps were run since that data is not available outside of run!!

tech4GT · 2018-06-07T18:42:03Z

I tried some techniques but it doesn't seem to work!!, should I try to output this as well that which steps have been run? That might need quite some change, should we open a separate issue for that?

Signed-off-by: tech4GT <[email protected]>

jywarren · 2018-06-07T18:59:42Z

Hmm, what if you used invert twice, and then used a very small data-url image (as we've used in other tests) and compare it to see if it's the inverted version or not? You could visually compare the first time, then you'd be able to see if the inverted version matches. So you'd only be looking at whether invert ran once or twice, with .run(1) vs. just .run() -- you know?

tech4GT · 2018-06-07T19:05:00Z

@jywarren I didn't quite get this... are you suggesting comparing the image data to find whether the image was inverted, like parsing the data and checking?

jywarren · 2018-06-07T19:11:53Z

Yes - check out this example!

image-sequencer/test/modules/image-manip.js

Lines 41 to 51 in a0f7e6c

    
           test("Twice inverted image is identical to original image", function (t) { 
        
             var step1 = sequencer.images.image1.steps[0].output.src; 
        
             var step3 = sequencer.images.image1.steps[2].output.src; 
        
             step1 = DataURItoBuffer(step1); 
        
             step3 = DataURItoBuffer(step3); 
        
             looksSame(step1,step3,function(err,res){ 
        
               if(err) console.log(err); 
        
               t.equal(res,true); 
        
               t.end(); 
        
             }); 
        
           });

tech4GT · 2018-06-07T19:13:29Z

@jywarren then I'll figure this out tomorrow maybe, it's pretty late, will that be ok?

jywarren · 2018-06-07T19:18:00Z

for sure! Sounds good Varun. Good night!

…

On Thu, Jun 7, 2018 at 3:14 PM Varun Gupta ***@***.***> wrote: @jywarren <https://github.com/jywarren> then I'll figure this out tomorrow maybe, it's pretty late, will that be ok> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#275 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ9m3lIGUEEyEMlDprBy7eIUmtftiks5t6XtagaJpZM4UXDG6> .

tech4GT · 2018-06-08T14:11:56Z

@jywarren Now working on this!!

tech4GT · 2018-06-08T14:30:08Z

@jywarren even if we do it this way how can we still compare? I mean sequencer.run() will run both of the steps and sequencer.run(1) will just not run the previous invert but still it will give the same output. My point being how can we distinguish between the 2 by a test if the output is same on both. We can include a runcount passed to run's callback, what say?

tech4GT · 2018-06-08T14:54:25Z

@jywarren I have something in mind for this but I am at airport so I'll try to submit the pr as soon as possible, basically I am thinking of removing one invert step from the sequence and then running the sequencer from after that removed step to shoe that the output does not get affected, how does that sound??

tech4GT · 2018-06-08T14:54:45Z

@jywarren I have something in mind for this but I am at airport so I'll try to submit the pr as soon as possible, basically I am thinking of removing one invert step from the sequence and then running the sequencer from after that removed step to show that the output does not get affected, how does that sound??

jywarren · 2018-06-08T14:57:21Z

Hm, i see what you mean. OK, your idea sounds good -- want to merge this or include that extra test in the PR?

…

On Fri, Jun 8, 2018 at 10:55 AM Varun Gupta ***@***.***> wrote: @jywarren <https://github.com/jywarren> I have something in mind for this but I am at airport so I'll try to submit the pr as soon as possible, basically I am thinking of removing one invert step from the sequence and then running the sequencer from after that removed step to shoe that the output does not get affected, how does that sound?? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#275 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ3dae7nfElLG62nUOmkqMmrD_E8Nks5t6pA7gaJpZM4UXDG6> .

tech4GT · 2018-06-08T14:59:40Z

@jywarren i am fine with either way, it should not take much time anyway🙂

jywarren · 2018-06-08T15:05:07Z

Cool, i'll wait then! Safe travels!

…

On Fri, Jun 8, 2018 at 11:00 AM Varun Gupta ***@***.***> wrote: @jywarren <https://github.com/jywarren> i am fine with either way, it should not take much time anyway🙂 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#275 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABfJ2G_JQ8iUVPCwayV2-u_ZhIS3lzhks5t6pFjgaJpZM4UXDG6> .

Signed-off-by: tech4GT <[email protected]>

tech4GT · 2018-06-08T17:59:11Z

@jywarren Done!! This is working nicely!!

jywarren · 2018-06-08T18:14:14Z

Fantastic work, Varun. This is a very readable, well tested self contained and very significant improvement!

tech4GT · 2018-06-08T18:16:46Z

Thanks a lot @jywarren This has been so exciting!!! let's complete the stepAPI next and close that issue and then move to meta modules??

jywarren · 2018-06-08T18:36:51Z

https://github.com/publiclab/image-sequencer/releases/tag/v2.0.0 :-) yes, that sounds good. Hopefully we don't have to break the API again on stepAPI so we won't need to so quickly bump another major version. But either way it'll be great!

tech4GT · 2018-06-08T18:38:48Z

haha, right!! I'll be more mindful about refactoring from now on!! Great!!😄

jywarren · 2018-06-08T18:39:30Z

Hmm, i'm seeing some oddities in the demo now that I've merged v2 - can you try it out? Just adding and deleting steps. Are we using the new run indices?

jywarren · 2018-06-08T18:41:02Z

oh no, it's not a problem to bump a major version number early in development. But as people begin to use our code, we'll want to aim for stability.

If there are some demo run/index changes we need to implement, maybe we can make them all consistent in #286 -- running addStep, deleteStep, and any settings update should update only the current step and ahead.

jywarren · 2018-06-08T18:41:21Z

I can wait to publish to NPM until we get that all ironed out!

tech4GT added 2 commits June 1, 2018 22:25

fixes publiclab#269

fe0b5c4

Signed-off-by: tech4GT <[email protected]>

update demo

ae1a4c3

Signed-off-by: tech4GT <[email protected]>

ghost assigned tech4GT Jun 1, 2018

ghost added the in-progress label Jun 1, 2018

jywarren reviewed Jun 1, 2018

View reviewed changes

refactor and comments

4801c71

Signed-off-by: tech4GT <[email protected]>

jywarren reviewed Jun 2, 2018

View reviewed changes

update readme

9cf8e04

Signed-off-by: tech4GT <[email protected]>

jywarren requested changes Jun 3, 2018

View reviewed changes

jywarren mentioned this pull request Jun 6, 2018

Fix import image #282

Merged

tech4GT added 3 commits June 7, 2018 19:17

merged with master

1c3823a

Signed-off-by: tech4GT <[email protected]>

updated and refactored

eeac9d0

Signed-off-by: tech4GT <[email protected]>

update readme

cac4d48

Signed-off-by: tech4GT <[email protected]>

1.8.0

b15e11c

jywarren reviewed Jun 7, 2018

View reviewed changes

update test

09021d9

Signed-off-by: tech4GT <[email protected]>

add test for run

dfbf0f5

Signed-off-by: tech4GT <[email protected]>

jywarren approved these changes Jun 8, 2018

View reviewed changes

jywarren merged commit 10c6ad5 into publiclab:master Jun 8, 2018

ghost removed the in-progress label Jun 8, 2018

Run optimize #275

Run optimize #275

Conversation

tech4GT commented Jun 1, 2018

jywarren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tech4GT commented Jun 2, 2018

tech4GT commented Jun 2, 2018

tech4GT commented Jun 2, 2018

jywarren commented Jun 2, 2018 via email

jywarren commented Jun 2, 2018

tech4GT commented Jun 2, 2018

jywarren commented Jun 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tech4GT Jun 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tech4GT commented Jun 2, 2018

tech4GT commented Jun 2, 2018

tech4GT commented Jun 2, 2018

tech4GT commented Jun 2, 2018

jywarren commented Jun 2, 2018 via email

tech4GT commented Jun 2, 2018

jywarren commented Jun 2, 2018 via email

tech4GT commented Jun 2, 2018

tech4GT commented Jun 3, 2018 • edited Loading

jywarren left a comment

Choose a reason for hiding this comment

jywarren Jun 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tech4GT commented Jun 3, 2018

tech4GT commented Jun 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tech4GT commented Jun 7, 2018

tech4GT commented Jun 7, 2018

jywarren commented Jun 7, 2018

tech4GT commented Jun 7, 2018

jywarren commented Jun 7, 2018

tech4GT commented Jun 7, 2018 • edited Loading

jywarren commented Jun 7, 2018 via email

tech4GT commented Jun 8, 2018

tech4GT commented Jun 8, 2018

tech4GT commented Jun 8, 2018

tech4GT commented Jun 8, 2018 • edited Loading

jywarren commented Jun 8, 2018 via email

tech4GT commented Jun 8, 2018

jywarren commented Jun 8, 2018 via email

tech4GT commented Jun 8, 2018

jywarren commented Jun 8, 2018

tech4GT commented Jun 8, 2018

jywarren commented Jun 8, 2018

tech4GT commented Jun 8, 2018

jywarren commented Jun 8, 2018

jywarren commented Jun 8, 2018

jywarren commented Jun 8, 2018

tech4GT Jun 3, 2018 •

edited

Loading

tech4GT commented Jun 3, 2018 •

edited

Loading

jywarren Jun 3, 2018 •

edited

Loading

tech4GT commented Jun 7, 2018 •

edited

Loading

tech4GT commented Jun 8, 2018 •

edited

Loading