-
Notifications
You must be signed in to change notification settings - Fork 40
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
updated czi-extract-plugin #491
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This look to be in pretty good shape. I have left comments for things that need to be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be formatted with proper markdown syntax. See here for a proper CHANGELOG format:
".ome.tif", | ||
) | ||
out_json["outDir"].append(out_name) | ||
json.dump(out_json, jfile, indent=2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When running with the preview
option, we should return
here and not run the rest of the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
||
data = s.data_segment().data().reshape(dims) | ||
|
||
write_thread(out_file_path, data, metadata, chan_names[c]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've done all the ground work for making this multithreaded. Please actually use preadator here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should be named conftest.py
. Then, it will not need to be explicitly imported in the tests because pytest
will handle it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
def clean_directories() -> None: | ||
"""Remove all temporary directories.""" | ||
for d in Path(".").cwd().iterdir(): | ||
if d.is_dir() and d.name.startswith("tmp"): | ||
shutil.rmtree(d) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the comment below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am downloading large czi files and dont want to download it again for every test. which is why I am cleaning directories after running tests
@pytest.fixture() | ||
def output_directory() -> Union[str, Path]: | ||
"""Create output directory.""" | ||
return Path(tempfile.mkdtemp(dir=Path.cwd())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not pass the dir
parameter here. tempfile
will automatically determine the correct place for the directory. Further, when the tests end (regardless of whether they succeeded or failed), the temp directories will automatically be cleaned up.
row_conv = dict( | ||
zip( | ||
np.unique(np.sort(ind["Y"])), | ||
range(0, len(np.unique(ind["Y"]))), | ||
), | ||
) | ||
col_conv = dict( | ||
zip( | ||
np.unique(np.sort(ind["X"])), | ||
range(0, len(np.unique(ind["X"]))), | ||
), | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Store the unique values in a variable, then sort them. Otherwise, this would be very slow for large archives.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
row=ind["Row"][i], | ||
col=ind["Col"][i], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do all CZI archives contain FOVs that have row and column information? If not, then this code could break.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now it is handled in a code if function is unable to extract unique fovs in czi file.
f0f3c69
to
594e140
Compare
This plugin is updated to latest plugin standards
Updated dependencies and included filepattern for file parsing
Wrote pytests for this plugin