search

isetbio / isetbio

Tools for modeling image systems engineering in the human visual system front end
MIT License
103 stars 42 forks source link

Ability to set seed for cone mosaic generation? #398

Closed tlian7 closed 2 years ago

tlian7 commented 5 years ago

When running my image metric calculations, I found it helpful to be able to fix the seed on the cone mosaic being generated. This way, I had the option to compare the same two cone mosaics for both the test and reference image.

I achieved this by adding an optional "patternSeed" parameters for the cone mosaic class. The alternate way to do this would be to generate a single cone mosaic object, and then re-use it for multiple calculations. However, the way I was looping and traversing through image metric databases made that method difficult, which is why I added the option to set the seed.

Do you guys think this might be a good addition to the class?

I note in the comments that the cone mosaic is only fixed for a single FOV, if you regenerate with a different size the cone mosaic will change even if the seed is fixed.

wandell commented 5 years ago

Always good to be able to set the seed for any random number computation. Please look at the wording and choices for the other cases in which we set the random seed and make sure it conforms to that. I can’t get to it while here. Perhaps Nicolas can help.

Brian

From: Gituhub Notifications notifications@github.com Reply-To: isetbio/isetbio reply@reply.github.com Date: Tuesday, July 30, 2019 at 6:26 AM To: isetbio/isetbio isetbio@noreply.github.com Cc: Subscribed subscribed@noreply.github.com Subject: [isetbio/isetbio] Ability to set seed for cone mosaic generation? (#398)

When running my image metric calculations, I found it helpful to be able to fix the seed on the cone mosaic being generated. This way, I had the option to compare the same two cone mosaics for both the test and reference image.

I achieved this by adding an optional "patternSeed" parameters for the cone mosaic class. The alternate way to do this would be to generate a single cone mosaic object, and then re-use it for multiple calculations. However, the way I was looping and traversing through image metric databases made that method difficult, which is why I added the option to set the seed.

Do you guys think this might be a good addition to the class?

I note in the comments that the cone mosaic is only fixed for a single FOV, if you regenerate with a different size the cone mosaic will change even if the seed is fixed.

You can view, comment on, or merge this pull request online at:

https://github.com/isetbio/isetbio/pull/398

Commit Summary

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/isetbio/isetbio/pull/398?email_source=notifications&email_token=AAOAQWJXDYJKKUVA6IIPJBDQB5OBJA5CNFSM4IHWLZS2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HCEWO4A, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AAOAQWI2MGVQUTLCUYWLSO3QB5OBJANCNFSM4IHWLZSQ.

DavidBrainard commented 5 years ago

I think it is fine to add this functionality and merge into the master branch.

How to handle the seed is in general tricky, and we have had trouble over the years with routines down deep resetting it and leading to unexpected results when someone thinks they have frozen it at some top level.

I think best practice in this case when you set the seed for this routine is to probably to get the current seed just before you set it, and restore that seed when you return. I think that will minimize interaction of your freezing the seed with other pieces of the code.

Best,

David

From: Brian Wandell notifications@github.com Reply-To: isetbio/isetbio reply@reply.github.com Date: Monday, July 29, 2019 at 5:39 PM To: isetbio/isetbio isetbio@noreply.github.com Cc: Subscribed subscribed@noreply.github.com Subject: Re: [isetbio/isetbio] Ability to set seed for cone mosaic generation? (#398)

Always good to be able to set the seed for any random number computation. Please look at the wording and choices for the other cases in which we set the random seed and make sure it conforms to that. I can’t get to it while here. Perhaps Nicolas can help.

Brian

From: Gituhub Notifications notifications@github.com Reply-To: isetbio/isetbio reply@reply.github.com Date: Tuesday, July 30, 2019 at 6:26 AM To: isetbio/isetbio isetbio@noreply.github.com Cc: Subscribed subscribed@noreply.github.com Subject: [isetbio/isetbio] Ability to set seed for cone mosaic generation? (#398)

When running my image metric calculations, I found it helpful to be able to fix the seed on the cone mosaic being generated. This way, I had the option to compare the same two cone mosaics for both the test and reference image.

I achieved this by adding an optional "patternSeed" parameters for the cone mosaic class. The alternate way to do this would be to generate a single cone mosaic object, and then re-use it for multiple calculations. However, the way I was looping and traversing through image metric databases made that method difficult, which is why I added the option to set the seed.

Do you guys think this might be a good addition to the class?

I note in the comments that the cone mosaic is only fixed for a single FOV, if you regenerate with a different size the cone mosaic will change even if the seed is fixed.

You can view, comment on, or merge this pull request online at:

https://github.com/isetbio/isetbio/pull/398

Commit Summary

File Changes

Patch Links:

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/isetbio/isetbio/pull/398?email_source=notifications&email_token=AAOAQWJXDYJKKUVA6IIPJBDQB5OBJA5CNFSM4IHWLZS2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HCEWO4A, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AAOAQWI2MGVQUTLCUYWLSO3QB5OBJANCNFSM4IHWLZSQ.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/isetbio/isetbio/pull/398?email_source=notifications&email_token=AAI24ZLPYSYKZ4EOOES63FDQB5POVA5CNFSM4IHWLZS2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3CCXCY#issuecomment-516172683, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AAI24ZLK3DZN5MHGBX44YBLQB5POVANCNFSM4IHWLZSQ.

npcottaris commented 5 years ago

Having a patternSeed can be useful. But as David says, it can be tricky in some instances. For example, some compute() methods (absorptions, photocurrent, eye movements) accept a seed parameter. So if you are in a loop and you are re-setting the seed there could be interactions. This becomes especially problematic if you are using parfor in your computations (as we do in the compute methods) because each worker has its own seed. So, depending on how your compute loop is organized setting the seed may or may not work. Obviously it works in your case, but it may fail in others. My preference for maximum assurance would be to save the mosaic and reload it whenever you want.