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

solution to Issue 70 & new options #72

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

trasse
Copy link

@trasse trasse commented Mar 10, 2020

No description provided.

@tibuch
Copy link
Collaborator

tibuch commented Mar 10, 2020

Hi @trasse,

Thank you for the PR. Could you please refactor your code such that we can merge the bug fix and new options into N2V_DataGenerator?

@trasse
Copy link
Author

trasse commented Mar 10, 2020

yes, i just updated the request!

Copy link
Collaborator

@tibuch tibuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great additions! Thank you!

But we have to make sure that the old methods behave the same, else we will break running notebooks with the next release.

I think we should leave load_imgs_from_directory as it is and add a new method load_imgs_from_directory2 which also returns the file-list. This will probably require an additional method load_imgs_from_file_list(files) which loads the images from a file-list. This last method will then be called by both methods. Like this we can have both versions and avoid duplicated code.

@@ -10,7 +10,7 @@ class N2V_DataGenerator():
The 'N2V_DataGenerator' enables training and validation data generation for Noise2Void.
"""

def load_imgs(self, files, dims='YX'):
def load_imgs(self, files, to32bit, dims='YX'):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This parameter should have a default value of False. This will ensure backwards compatibility.

@@ -80,7 +84,7 @@ def load_imgs(self, files, dims='YX'):

return imgs

def load_imgs_from_directory(self, directory, filter='*.tif', dims='YX'):
def load_imgs_from_directory(self, directory, filter='*.tif', dims='YX', names_back = False, to32bit = True):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to32bit = False for backwards compatibility.

"""

files = glob(join(directory, filter))
files.sort()
return self.load_imgs(files, dims=dims)
if names_back:
return files, self.load_imgs(files, to32bit, dims=dims)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure what the best practice would be here. Since this changes the return value of this method.

I think the best would be to refactor the code, such that we have two methods. The old one as it is and a new one which also returns the file list. Both these methods should probably wrap a new third method which takes a file-list and return the image-list.

I would also put the files as second return parameter, then the user always gets the image-list as first return value.

print('Generated patches:', patches.shape)
if shuffle_patches:
np.random.shuffle(patches)
#print('Generated patches:', patches.shape)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the reason to remove this print statement?

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.

2 participants