search

holoviz-topics / imagen

ImaGen: Generic Python library for 0D, 1D and 2D pattern distributions
https://imagen.holoviz.org/
BSD 3-Clause "New" or "Revised" License
31 stars 16 forks source link

fixed caching bug and related issues #36

Closed spiglerg closed 10 years ago

jlstevens commented 10 years ago

The code is a big improvement over the last PR to fix this issues.

Once you confirm that you can reset the various channel data lists using a new empty list (instead of emptying in place), I think this can be merged.

There are a few minor issues that I am happy to deal this (e.g. stray commented code) once merged.

spiglerg commented 10 years ago

I'll fix this and it will be mergeable later this afternoon (on my next commit). :)

On Sat, Aug 2, 2014 at 2:01 PM, J.L Stevens notifications@github.com wrote:

The code is a big improvement over the last PR for this. Once you confirm that you can reset the various channel data lists using a new empty list (instead of emptying in place), I think this can be merged.

There are a few minor issues that I am happy to deal this (e.g. stray commented code) once merged.

— Reply to this email directly or view it on GitHub https://github.com/ioam/imagen/pull/36#issuecomment-50962253.

jlstevens commented 10 years ago

Your reply to Jim is now hidden - it is attached to the wrong line on an outdated diff. I'll reproduce it here for you:

Because self() from Selector already generates new data from current_generator, and channels() repeats the exact same operation. It's a waste of computing time.

In the future we could add support for use_cached in Selector too: however it has to be designed carefully, as no generator might be selected and this could lead to crashes.

If you want, I am happy to apply your diff directly to master once this is decided (and credit you appropriately). Your commit messages are completely useless and I see no good reason for them to appear in the commit history.

spiglerg commented 10 years ago

Oh right, thank you very much ! :)

On Sat, Aug 2, 2014 at 4:54 PM, J.L Stevens notifications@github.com wrote:

Your reply to Jim is now hidden - it is attached to the wrong line on an outdated diff. I'll reproduce it here for you:

Because self() from Selector already generates new data from current_generator, and channels() repeats the exact same operation. It's a waste of computing time.

In the future we could add support for use_cached in Selector too: however it has to be designed carefully, as no generator might be selected and this could lead to crashes.

—

Reply to this email directly or view it on GitHub https://github.com/ioam/imagen/pull/36#issuecomment-50966363.

jbednar commented 10 years ago

Ok, based on Giacomo's explanation, the code for Selector.channels seems fine, though it needs a comment explaining this fairly convoluted reasoning. So, Jean-Luc, you can apply these changes and then close this PR if you like, or just merge it and take the hit on those three not-very-informative commit messages if you want to be lazy like I would be. Thanks!

spiglerg commented 10 years ago

Just added a quick explanation in the docstring!

On Sat, Aug 2, 2014 at 9:34 PM, James A. Bednar notifications@github.com wrote:

Ok, based on Giacomo's explanation, the code for Selector.channels seems fine, though it needs a comment explaining this fairly convoluted reasoning. So, Jean-Luc, you can apply these changes and then close this PR if you like, or just merge it and take the hit on those three not-very-informative commit messages if you want to be lazy like I would be. Thanks!

— Reply to this email directly or view it on GitHub https://github.com/ioam/imagen/pull/36#issuecomment-50973578.

jlstevens commented 10 years ago

Applied these fixes in ceaa5e7. Thanks!