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

Refurbishment #118

Merged
merged 3 commits into from
Apr 4, 2024
Merged

Refurbishment #118

merged 3 commits into from
Apr 4, 2024

Conversation

kostrykin
Copy link
Member

Now that image-based test verification is supported, it is time to refurbish our tools and eliminate bugs or inconsistencies which before went undetected.

The following tools are updated in this PR:

  • 2d_auto_threshold
  • 2d_feature_extraction
  • 2d_filter_segmentation_by_features
  • 2d_histogram_equalization
  • 2d_simple_filter
  • anisotropic_diffusion
  • bfconvert
  • bioformats2raw
  • colorize_labels
  • concat_channels
  • image_math
  • morphological_operations
  • orientationpy
  • overlay_images
  • projective_transformation
  • projective_transformation_points
  • rfove
  • scale_image
  • segmetrics
  • slice_image
  • split_labelmap
  • superdsm
  • voronoi_tessellation

To avoid boilerplate code and provide meaningful defaults for different types of images, macros for testing are defined. These are documented in CONTRIBUTING.md.

In addition, on the occasion of the updates, the help sections of many tools have been revised, and the creators of the tool wrappers have been added.

The following bugs and inconsistencies were fixed:

  • The tools scale_image and 2d_simple_filter will now preserve the brightness and the range of values of images (for 2d_simple_filter this only concerns mean filters like Gaussian and median). This will be handy for such applications.
  • The tool scale_image now supports TIFF files.
  • The tool concat_images now supports either preserving the range of values or the image brightness.
  • Some other minor fixes.

Last but not least, the CI is updated to also support the TIFF file features added in galaxyproject/galaxy#17797.

* Rename tools

Change the `Performs` in the name of the two tools

- `Performs projective transformation with/without labels`
- `Performs projective transformation`

to `Perform`, which is more consistent with the name of the other tools (imperative form).

* Update projective_transformation.xml

* Update projective_transformation_points.xml

* Add CONVENTIONS.md

* wip

* wip

* Update CONVENTIONS.md

* Update CONVENTIONS.md

* Update CONVENTIONS.md

* Update CONVENTIONS.md

* Update CONVENTIONS.md

* Update CONVENTIONS.md

* Remove help section

* Update CONVENTIONS.md

* Update CONVENTIONS.md

* Update CONVENTIONS.md

* Update

* Update test data

* Add content assertions

* Update pr.yaml

* Update

* Update pr.yaml

* Add `pip install pillow`

* Merge CONVENTIONS.md into CONTRIBUTING.md

* Temporarilly fix CI

* Update `2d_feature_extraction` tool

* Update `2d_auto_threshold` tool

* Fix `2d_feature_extraction` tests

* Fix test data

* Fix tool

* Start refactoring `2d_simple_filter` tool

* Fix `2d_simple_filter` tool

* Add more tests

* Fix tests

* Update

* Fix

* Update CONTRIBUTING.md

* Fix bugs

* Update CONTRIBUTING.md

* Update CONTRIBUTING.md

* Fix linting issues

* Update CONTRIBUTING.md

* Refactor tests using macros

* Update tests

* Update CONTRIBUTING.md

* Update CONTRIBUTING.md

* Update pr.yaml

* Update `anisotropic_diffusion` tool

* Fix spelling (was BE, using AE is more consistent)

* Update `2d_histogram_equalization` tool

* Update `colorize_labels` tool

* Update `bf2raw` tool

* Add `macros/creators.xml`

* Update `rfove` tool

* Fix `bioformats2raw` and `voronoi_tessellation` tools

* Fix labels

* Fix `rfove` tool

* Update `overlay_images` tool

* Add RGB test for `overlay_images`

* Update `overlay_images` help

* Add image verification support to CI (BMCV#117)

* Update pr.yaml

* Update

* Update pr.yaml

* Add `pip install pillow`

* Temporarilly fix CI

* Update pr.yaml

* Update ci.yaml

* Add test tool from `/test/functional/tools/image_diff.xml`

* Update pr.yaml

* Add .shed.yml

* Add missing test files

* Switch CI to galaxy fork `kostrykin/galaxy` branch `galaxy-image-analysis`

This is a temporary change to enable the features from:

 - galaxyproject/galaxy#17556
 - galaxyproject/galaxy#17581

* Remove `test` tool

* Update CONTRIBUTING.md

* Update CONTRIBUTING.md

* Update CONTRIBUTING.md

* Update test data

* Add RGB test for `overlay_images`

* Update `anisotropic_diffusion` to medpy 0.4.0

* Update PULL_REQUEST_TEMPLATE.md

* Update `projective_transformation_points` tool

* Update PULL_REQUEST_TEMPLATE.md

* Update `bioformats2raw`

* Update `projective_transformation` tool

* Update `bfconvert` tool (tests failing)

* Update CONTRIBUTING.md

* Fix `bfconvert` tests

* Fix test data size

* Update `projective_transformation_points` tool

* Remove spurious files

* Update requirements using `@TOOL_VERSION@` macro

* Update `segmetrics` tool

* Remove spurious sym links

* Update `superdsm` tool

* Update `scale_image` tool (not finished yet)

* Fix tests

* Fix `scale_image` tool

* Refactor

* Fix linting issues

* Update tests and help of `2d_simple_filter` tool

* Update `scale_image` help

* Update CI to use latest version of `galaxy` fork

* Fix tests for float TIFF in `scale_image` tool

* Add tifffile to CI workflows

* Fix tests of `bfconvert` tool

* Update `2d_filter_segmentation_by_features` tool

* Update `image_math` tool

* Update `orientationpy` tool

* Update `morphological_operations` tool

* Update `split_labelmap` tool

* Add float TIFF test for `2d_simple_filter`

* Update `slice_image` tool

* Add missing macros

* Update help of `2d_filter_segmentation_by_features` tool

* Update `concat_images` tool

* Fix linting issues

* Fix file sizes

* Fix missing `profile`
Copy link
Collaborator

@bgruening bgruening left a comment

Choose a reason for hiding this comment

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

Very cool, a bunch of very nice changes. A few comments inside.

@@ -0,0 +1,23 @@
<macros>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please note that the entire file is shipped to all tools.

@@ -0,0 +1,93 @@
<macros>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't this make it very unreadable for the actual tool?
And also here those macro files are shipped to all tools that import them, even if they do not use most of the stuff ... and it gets parsed and loaded into Galaxy I think.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm torn, I like how compact it is, but I'm worried those files are growing and growing and single tools don't use much of it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the feedback!

On the other hand, if the macros were split into more fine-grained files, this would result in more I/O operations for loading the tools. Do you think that this is tolerable and parsing a few superfluous macros is worse?

My concern is due to: I remember from my C++ days, that, for compilers, I/O operations are the reason while compile times are long. By concatenating all files into a single file (aka unitybuild), compile times could be reduced by >90% (the improvement was somewhat less when using SSD instead of HDD but still huge).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand, but I guess this is not whats happening here. Every tool will ship the macro, so the number of files is the same in both cases. I one scenario its just a lot small files, and in your case its potentially a lot of big files with a lot of information that is not used for individual tools.

Its an academic discussion, please keep it like it is, I just wanted to point out this disadvantage.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is 0 Bytes and other as well, is that ok?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

True, sorry fo the noise.

@kostrykin kostrykin marked this pull request as draft March 30, 2024 21:10
@kostrykin kostrykin marked this pull request as ready for review April 4, 2024 12:53
@kostrykin kostrykin merged commit c045f06 into BMCV:master Apr 4, 2024
13 checks passed
@bgruening
Copy link
Collaborator

Very cool!

@kostrykin kostrykin deleted the overhaul/upstream branch September 24, 2024 11:19
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