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

fixed reading TIFF real valued intensities in range 0-1 as integer 0 #155

Conversation

friskluft
Copy link
Member

No description provided.

@sweep-ai
Copy link

sweep-ai bot commented Oct 23, 2023

Apply Sweep Rules to your PR?

  • Apply: Leftover TODOs in the code should be handled.
  • Apply: All new business logic should have corresponding unit tests in the tests/ directory.
  • Apply: Any clearly inefficient or repeated code should be optimized or refactored.

@@ -40,7 +41,10 @@ bool ImageLoader1x::open(const std::string& fpath)
else
{
if (Nyxus::check_tile_status(fpath))
FL = std::make_unique<NyxusGrayscaleTiffTileLoader<uint32_t>> (n_threads, fpath);
FL = std::make_unique<NyxusGrayscaleTiffTileLoader<uint32_t>> (n_threads, fpath,
Copy link
Member

@sameeul sameeul Nov 2, 2023

Choose a reason for hiding this comment

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

Why are we passing in globals this way instead of modifying the ImageLoader1x constructor?

@@ -165,10 +174,10 @@ class NyxusGrayscaleTiffTileLoader : public AbstractTileLoader<DataType>
case 8:
case 16:
case 32:
loadTile<float>(tiffTile, tileDataVec);
loadTile_real_intens <float> (tiffTile, tileDataVec);
Copy link
Member

Choose a reason for hiding this comment

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

can we do something like

if (image_options_are_present) {
  loadTile_real_intens(...)
} else {
  loadTile(...)
}

That way, if we are not dealing with already scaled image (which is not the common case, we don't spend tile inspecting each pixel value while copying.

Copy link
Member Author

Choose a reason for hiding this comment

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

Floating point image options (in the form of expected minimum, maximum, and equivalent integer range) are always present, aren't they? And doing your suggested way, we'd need to do a separate pre-scan through an image to figure out its intensity range, that is also a time expense.

@@ -41,7 +44,12 @@ bool ImageLoader::open(const std::string& int_fpath, const std::string& seg_fpat
{
if (Nyxus::check_tile_status(int_fpath))
{
intFL = new NyxusGrayscaleTiffTileLoader<uint32_t>(n_threads, int_fpath);
intFL = new NyxusGrayscaleTiffTileLoader<uint32_t> (
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using globals here, can we modify the open() to pass the fimageOptions?

Copy link
Member Author

@friskluft friskluft Nov 6, 2023

Choose a reason for hiding this comment

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

My motive was to access the global settings as close to the point of use as possible, as just one control flow branch of many passes through the floating point pixels/voxels manipulation. Otherwise we would need to prepare those global settings to pass as parameters early amid the high-level code of phase 2 (trivials) and 3 (non-trivials) image processing logic where we are not concerned about byte layouts in pixels, and do it twice - in function processDataset() and processNontrivialRois(). Your opinion?

@sameeul sameeul merged commit c00ad8a into PolusAI:main Nov 6, 2023
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