WIP: Added IndexRange for efficient region and grid space iteration #106
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Added Experimental::IndexRange, to allow efficient iteration over
the indices of an image region or grid space by means of a C++11
range-based for-loop or Standard C++ algorithm.
Appears useful in combination with ShapedImageNeighborhoodRange, e.g.:
IndexRange<Dimension> indexRange{ imageSize };
for (IndexType index : indexRange)
{
shapedImageNeighborhoodRange.SetLocation(index);
for (PixelType neighborPixel : shapedImageNeighborhoodRange)
// Process neighbor pixel...
}
Added ImageRegionIndexRange<Dimension> alias for region iteration.
Change-Id: Ia1f777ba251a9c617f55c16bf37878eaee632f43
N-Dekker
reviewed
Nov 6, 2018
| */ | ||
| template <typename TDummy = typename std::enable_if<!VBeginAtZero, std::nullptr_t>::type> | ||
| explicit IndexRange(const ImageRegion<VDimension>& imageRegion, TDummy = nullptr) | ||
| : |
There was a problem hiding this comment.
This is not the correct way to apply SFINAE (my bad!), as my LKEB/LUMC colleague Baldur van Lew pointed out. The correct way is described at https://stackoverflow.com/questions/46603353/sfinae-on-constructors-works-in-vc2017-but-not-in-clang-gcc
There was a problem hiding this comment.
Will you be updating this to the correct way?
There was a problem hiding this comment.
@hjmjohnson I would certainly like to fix this SFINAE attempt, but I first have to figure out how to update a GitHub pull request from a fork of yours!
|
@hjmjohnson Same question as for #105 : can you please explain why you closed this PR? |
|
Accidently deleted/closed branch when the script for moving from Gerrit went haywire. |
|
Blowing my mind @N-Dekker ! 💥 Love it ❤️ |
|
You can’t update mine. Just fetch the PR locally, and then make a new PR.
This is one advantage of gerrit.
…Sent from my iPhone
On Nov 7, 2018, at 7:46 AM, Niels Dekker ***@***.***> wrote:
@N-Dekker commented on this pull request.
In Modules/Core/Common/include/itkIndexRange.h:
> + :
+ // Note: Use parentheses instead of curly braces to initialize data members,
+ // to avoid AppleClang 6.0.0.6000056 compile errors, "no viable conversion..."
+ m_MinIndex(),
+ m_MaxIndex(CalculateMaxIndex({}, size))
+ {
+ }
+
+
+ /** Constructs a range of indices for the specified image region.
+ * \note This function is unavailable when VBeginAtZero is true (in
+ * which case, it is removed by means of C++ SFINAE).
+ */
+ template <typename TDummy = typename std::enable_if<!VBeginAtZero, std::nullptr_t>::type>
+ explicit IndexRange(const ImageRegion<VDimension>& imageRegion, TDummy = nullptr)
+ :
@hjmjohnson I would certainly like to fix this SFINAE attempt, but I first have to figure out how to update a GitHub pull request from a fork of yours!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
You can open a merge request into @hjmjohnson branch in his fork, but way too convoluted! hehe @N-Dekker This is great! Are you running any benchmark? It would be cool to have data about the improvements you are doing in Experimental! |
Thank, Pablo! So far I only did a benchmark to check if it's useful to distinguish at compile-time between index iteration that starts at index [0, 0] (only parameterized by the image size, |
@thewtex Sorry Matt, didn't want to blow your mind. I really wanted to keep it simple! However, I felt challenged to add support for index iteration within an arbitrary |
|
Please continue further review at the PR from my fork: 👉 #155 |
|
Superceeded by #155 |
2 tasks
hjmjohnson
added a commit
to hjmjohnson/ITK
that referenced
this pull request
Dec 26, 2020
Fix typos in comments CMakeLists.txt: Export all symbols (InsightSoftwareConsortium#136) test/cctest/CMakeLists.txt: Added /bigobj for MSVC tests (InsightSoftwareConsortium#135) Add DOUBLE_CONVERSION_HAS_ATTRIBUTE to fix warnings on MSVC and enable for GCC. (InsightSoftwareConsortium#131) Fix broken MSVC builds. (InsightSoftwareConsortium#130) Add support for quiet and signaling NaNs to the ieee header. (InsightSoftwareConsortium#128) Move buffer and buffer_pos down (InsightSoftwareConsortium#125) * Move buffer and buffer_pos down Simplifies code by removing two asserts Optimize code with -ftrivial-auto-var-init=pattern by avoiding initialization when buffer is not used * Disable -ftrivial-auto-var-init=pattern for a large buffer Fix strtod.cc undefined constants (InsightSoftwareConsortium#123) When DOUBLE_CONVERSION_CORRECT_DOUBLE_OPERATIONS is not defined there is a build error when -Wall -Werror enabled because kMaxExactDoubleIntegerDecimalDigits, exact_powers_of_ten and kExactPowersOfTenSize are used only in else branch of this define (when it's defined). Fixes google/double-conversion#122 Add full license to test .cc files missing it. (InsightSoftwareConsortium#121) Some files in cctest had a 1-line copyright that doesn't list the license. This copies the project license to those files, preserving the date. Add wasm32 as supported platform (InsightSoftwareConsortium#120) Summary: Emscripten is already included, adding wasm32 the same way for when build with "plain" clang wasm32 (without emscripten) Pseiderer/add nios2 and xtensa 001 (InsightSoftwareConsortium#119) * double-conversion: enable for nios2 Nios2 supports double conversion, tested using qemu: Add support for microblaze. Add support for e2k architecture. (InsightSoftwareConsortium#118) Add min exponent width option in double-to-string conversion (InsightSoftwareConsortium#116) Remove reference to `diy-fp.cc` That file was removed in a previous commit. More Bignum fiddling. (InsightSoftwareConsortium#108) * Char has size 1. * More const. * Allow inlining. * Compact Bignum sizes. * Consistent naming. Remove redundant parenthesis. Reported by seanm (github). Optimise Bignum layout. (InsightSoftwareConsortium#107) * Use memset to clear bignum. * Improve data locality. * Reduce size of bignum. Split Strtod() (InsightSoftwareConsortium#106) Add `StrtodTrimmed` method, exposing a later stage of the conversion pipeline. Some tools can do the first stage outside of the double-conversion library and would prefer not to pay the cost of doing it again. Split double-conversion. (InsightSoftwareConsortium#104) Separates the two main classes into separate c and h files. Fix naming. (InsightSoftwareConsortium#103) Fix naming of `case_insensibility` to `case_insensitivity`. Consistent macro prefix. (InsightSoftwareConsortium#101) Use standard min/max. (InsightSoftwareConsortium#102) Fix some issues with invalid hex-float literals. When converting `0x` the converter would assert (or access out of boundary). With `0x1.p1234556666FFFFF` the converter would overflow and not yield the correct exponent. Usefulcat master (InsightSoftwareConsortium#98) * minor bug fix: use free() instead of delete[] to free memory allocated by strdup() * fix for uninitialized variable problem found by valgrind * disable assertions for 'optimize' build * removed -g option that was inadvertently added to CCFLAGS * ignore generated files Fix warning for g++ 4.9.3. CMake: install to correct lib dir (InsightSoftwareConsortium#93) 64-bit libraries should be installed in /usr/lib64, not in /usr/lib/ Make the destination lib dir configurable. Add big endian ARM support (InsightSoftwareConsortium#92) This fixes compilation on such platforms. Switch to relative includes. Fix 16-bit separators. msvc: check if _MSC_VER is defined (InsightSoftwareConsortium#88) Allow for compilation in emscripten (InsightSoftwareConsortium#86) Support emscripten Add test cases. Fixes InsightSoftwareConsortium#62 Add support of ARC architecture (InsightSoftwareConsortium#82) More info about ARC architecture is here: [1] & [2]. We need ARC supported here for many things like: - ICU (see [3]) - Qt5 etc Fix hex literal bug. Large hex literals would lose their minus sign. Support separator characters. Add support for hexadecimal float literals. Fix bug where hex numbers would lose the minus sign. Add comments for achitecture check. This should make it easier to add new architectures. Add support for aarch64_be, or1k and microblazebe. Add support for Windows on ARM and ARM64 (InsightSoftwareConsortium#76) Use `static_assert` with newer compilers. Add Native Client as support architecture. Avoid undefined cast to make ASAN happy. Avoid undefined cast to make ASAN happy. Add `exports_files` Processed length should include no trailing junk (InsightSoftwareConsortium#63) Clarify output charset in DoubleToAscii documentation (InsightSoftwareConsortium#61) * Clarify output charset in DoubleToAscii documentation * Fixing typo in charset docs. Fix warning for code that will never be executed (InsightSoftwareConsortium#59) Rename macros. Renamed DISALLOW_COPY_AND_ASSIGN and DISALLOW_IMPLICIT_CONSTRUCTORS to DC_DISALLOW_COPY_AND_ASSIGN and DC_DISALLOW_IMPLICIT_CONSTRUCTORS. This makes it easier to use this library in projects that have macros with the same name (as is common in Google). Some refactorings: remove unused static, replace deprecated headers, init member in constructor REF: replace deprecated headers REF: meaningless static definition in anonymous namespace REF: init member in constructor Fix typo in variable name. Suppress issue in clang analyzer. CMake fixes. Remove unused CMake file. Implement ALLOW_CASE_INSENSIBILITY mode for StringToDoubleConverter c… Remove unnecessary INSTALL_INTERFACE expression. Use template for CMake installation. Fix mistake for build interface include dir. Improve CMake changes. Update CMake package generation. Implement ALLOW_CASE_INSENSIBILITY mode for StringToDoubleConverter class Add assert and test. Avoid negative shift. When filling in fractional digits in a fixed representation we might use all existing digits. When this happens, we can not look at the next digit to see if we should round up. Before this fix, we tried to shift by a negative amount to see if the (non-existing) next bit was set to 1 (requiring the number to be rounded up).
hjmjohnson
added a commit
to hjmjohnson/ITK
that referenced
this pull request
Dec 26, 2020
Fix typos in comments CMakeLists.txt: Export all symbols (InsightSoftwareConsortium#136) test/cctest/CMakeLists.txt: Added /bigobj for MSVC tests (InsightSoftwareConsortium#135) Add DOUBLE_CONVERSION_HAS_ATTRIBUTE to fix warnings on MSVC and enable for GCC. (InsightSoftwareConsortium#131) Fix broken MSVC builds. (InsightSoftwareConsortium#130) Add support for quiet and signaling NaNs to the ieee header. (InsightSoftwareConsortium#128) Move buffer and buffer_pos down (InsightSoftwareConsortium#125) * Move buffer and buffer_pos down Simplifies code by removing two asserts Optimize code with -ftrivial-auto-var-init=pattern by avoiding initialization when buffer is not used * Disable -ftrivial-auto-var-init=pattern for a large buffer Fix strtod.cc undefined constants (InsightSoftwareConsortium#123) When DOUBLE_CONVERSION_CORRECT_DOUBLE_OPERATIONS is not defined there is a build error when -Wall -Werror enabled because kMaxExactDoubleIntegerDecimalDigits, exact_powers_of_ten and kExactPowersOfTenSize are used only in else branch of this define (when it's defined). Fixes google/double-conversion#122 Add full license to test .cc files missing it. (InsightSoftwareConsortium#121) Some files in cctest had a 1-line copyright that doesn't list the license. This copies the project license to those files, preserving the date. Add wasm32 as supported platform (InsightSoftwareConsortium#120) Summary: Emscripten is already included, adding wasm32 the same way for when build with "plain" clang wasm32 (without emscripten) Pseiderer/add nios2 and xtensa 001 (InsightSoftwareConsortium#119) * double-conversion: enable for nios2 Nios2 supports double conversion, tested using qemu: Add support for microblaze. Add support for e2k architecture. (InsightSoftwareConsortium#118) Add min exponent width option in double-to-string conversion (InsightSoftwareConsortium#116) Remove reference to `diy-fp.cc` That file was removed in a previous commit. More Bignum fiddling. (InsightSoftwareConsortium#108) * Char has size 1. * More const. * Allow inlining. * Compact Bignum sizes. * Consistent naming. Remove redundant parenthesis. Reported by seanm (github). Optimise Bignum layout. (InsightSoftwareConsortium#107) * Use memset to clear bignum. * Improve data locality. * Reduce size of bignum. Split Strtod() (InsightSoftwareConsortium#106) Add `StrtodTrimmed` method, exposing a later stage of the conversion pipeline. Some tools can do the first stage outside of the double-conversion library and would prefer not to pay the cost of doing it again. Split double-conversion. (InsightSoftwareConsortium#104) Separates the two main classes into separate c and h files. Fix naming. (InsightSoftwareConsortium#103) Fix naming of `case_insensibility` to `case_insensitivity`. Consistent macro prefix. (InsightSoftwareConsortium#101) Use standard min/max. (InsightSoftwareConsortium#102) Fix some issues with invalid hex-float literals. When converting `0x` the converter would assert (or access out of boundary). With `0x1.p1234556666FFFFF` the converter would overflow and not yield the correct exponent. Usefulcat master (InsightSoftwareConsortium#98) * minor bug fix: use free() instead of delete[] to free memory allocated by strdup() * fix for uninitialized variable problem found by valgrind * disable assertions for 'optimize' build * removed -g option that was inadvertently added to CCFLAGS * ignore generated files Fix warning for g++ 4.9.3. CMake: install to correct lib dir (InsightSoftwareConsortium#93) 64-bit libraries should be installed in /usr/lib64, not in /usr/lib/ Make the destination lib dir configurable. Add big endian ARM support (InsightSoftwareConsortium#92) This fixes compilation on such platforms. Switch to relative includes. Fix 16-bit separators. msvc: check if _MSC_VER is defined (InsightSoftwareConsortium#88) Allow for compilation in emscripten (InsightSoftwareConsortium#86) Support emscripten Add test cases. Fixes InsightSoftwareConsortium#62 Add support of ARC architecture (InsightSoftwareConsortium#82) More info about ARC architecture is here: [1] & [2]. We need ARC supported here for many things like: - ICU (see [3]) - Qt5 etc Fix hex literal bug. Large hex literals would lose their minus sign. Support separator characters. Add support for hexadecimal float literals. Fix bug where hex numbers would lose the minus sign. Add comments for achitecture check. This should make it easier to add new architectures. Add support for aarch64_be, or1k and microblazebe. Add support for Windows on ARM and ARM64 (InsightSoftwareConsortium#76) Use `static_assert` with newer compilers. Add Native Client as support architecture. Avoid undefined cast to make ASAN happy. Avoid undefined cast to make ASAN happy. Add `exports_files` Processed length should include no trailing junk (InsightSoftwareConsortium#63) Clarify output charset in DoubleToAscii documentation (InsightSoftwareConsortium#61) * Clarify output charset in DoubleToAscii documentation * Fixing typo in charset docs. Fix warning for code that will never be executed (InsightSoftwareConsortium#59) Rename macros. Renamed DISALLOW_COPY_AND_ASSIGN and DISALLOW_IMPLICIT_CONSTRUCTORS to DC_DISALLOW_COPY_AND_ASSIGN and DC_DISALLOW_IMPLICIT_CONSTRUCTORS. This makes it easier to use this library in projects that have macros with the same name (as is common in Google). Some refactorings: remove unused static, replace deprecated headers, init member in constructor REF: replace deprecated headers REF: meaningless static definition in anonymous namespace REF: init member in constructor Fix typo in variable name. Suppress issue in clang analyzer. CMake fixes. Remove unused CMake file. Implement ALLOW_CASE_INSENSIBILITY mode for StringToDoubleConverter c… Remove unnecessary INSTALL_INTERFACE expression. Use template for CMake installation. Fix mistake for build interface include dir. Improve CMake changes. Update CMake package generation. Implement ALLOW_CASE_INSENSIBILITY mode for StringToDoubleConverter class Add assert and test. Avoid negative shift. When filling in fractional digits in a fixed representation we might use all existing digits. When this happens, we can not look at the next digit to see if we should round up. Before this fix, we tried to shift by a negative amount to see if the (non-existing) next bit was set to 1 (requiring the number to be rounded up).
hjmjohnson
added a commit
that referenced
this pull request
Dec 28, 2020
Fix typos in comments CMakeLists.txt: Export all symbols (#136) test/cctest/CMakeLists.txt: Added /bigobj for MSVC tests (#135) Add DOUBLE_CONVERSION_HAS_ATTRIBUTE to fix warnings on MSVC and enable for GCC. (#131) Fix broken MSVC builds. (#130) Add support for quiet and signaling NaNs to the ieee header. (#128) Move buffer and buffer_pos down (#125) * Move buffer and buffer_pos down Simplifies code by removing two asserts Optimize code with -ftrivial-auto-var-init=pattern by avoiding initialization when buffer is not used * Disable -ftrivial-auto-var-init=pattern for a large buffer Fix strtod.cc undefined constants (#123) When DOUBLE_CONVERSION_CORRECT_DOUBLE_OPERATIONS is not defined there is a build error when -Wall -Werror enabled because kMaxExactDoubleIntegerDecimalDigits, exact_powers_of_ten and kExactPowersOfTenSize are used only in else branch of this define (when it's defined). Fixes google/double-conversion#122 Add full license to test .cc files missing it. (#121) Some files in cctest had a 1-line copyright that doesn't list the license. This copies the project license to those files, preserving the date. Add wasm32 as supported platform (#120) Summary: Emscripten is already included, adding wasm32 the same way for when build with "plain" clang wasm32 (without emscripten) Pseiderer/add nios2 and xtensa 001 (#119) * double-conversion: enable for nios2 Nios2 supports double conversion, tested using qemu: Add support for microblaze. Add support for e2k architecture. (#118) Add min exponent width option in double-to-string conversion (#116) Remove reference to `diy-fp.cc` That file was removed in a previous commit. More Bignum fiddling. (#108) * Char has size 1. * More const. * Allow inlining. * Compact Bignum sizes. * Consistent naming. Remove redundant parenthesis. Reported by seanm (github). Optimise Bignum layout. (#107) * Use memset to clear bignum. * Improve data locality. * Reduce size of bignum. Split Strtod() (#106) Add `StrtodTrimmed` method, exposing a later stage of the conversion pipeline. Some tools can do the first stage outside of the double-conversion library and would prefer not to pay the cost of doing it again. Split double-conversion. (#104) Separates the two main classes into separate c and h files. Fix naming. (#103) Fix naming of `case_insensibility` to `case_insensitivity`. Consistent macro prefix. (#101) Use standard min/max. (#102) Fix some issues with invalid hex-float literals. When converting `0x` the converter would assert (or access out of boundary). With `0x1.p1234556666FFFFF` the converter would overflow and not yield the correct exponent. Usefulcat master (#98) * minor bug fix: use free() instead of delete[] to free memory allocated by strdup() * fix for uninitialized variable problem found by valgrind * disable assertions for 'optimize' build * removed -g option that was inadvertently added to CCFLAGS * ignore generated files Fix warning for g++ 4.9.3. CMake: install to correct lib dir (#93) 64-bit libraries should be installed in /usr/lib64, not in /usr/lib/ Make the destination lib dir configurable. Add big endian ARM support (#92) This fixes compilation on such platforms. Switch to relative includes. Fix 16-bit separators. msvc: check if _MSC_VER is defined (#88) Allow for compilation in emscripten (#86) Support emscripten Add test cases. Fixes #62 Add support of ARC architecture (#82) More info about ARC architecture is here: [1] & [2]. We need ARC supported here for many things like: - ICU (see [3]) - Qt5 etc Fix hex literal bug. Large hex literals would lose their minus sign. Support separator characters. Add support for hexadecimal float literals. Fix bug where hex numbers would lose the minus sign. Add comments for achitecture check. This should make it easier to add new architectures. Add support for aarch64_be, or1k and microblazebe. Add support for Windows on ARM and ARM64 (#76) Use `static_assert` with newer compilers. Add Native Client as support architecture. Avoid undefined cast to make ASAN happy. Avoid undefined cast to make ASAN happy. Add `exports_files` Processed length should include no trailing junk (#63) Clarify output charset in DoubleToAscii documentation (#61) * Clarify output charset in DoubleToAscii documentation * Fixing typo in charset docs. Fix warning for code that will never be executed (#59) Rename macros. Renamed DISALLOW_COPY_AND_ASSIGN and DISALLOW_IMPLICIT_CONSTRUCTORS to DC_DISALLOW_COPY_AND_ASSIGN and DC_DISALLOW_IMPLICIT_CONSTRUCTORS. This makes it easier to use this library in projects that have macros with the same name (as is common in Google). Some refactorings: remove unused static, replace deprecated headers, init member in constructor REF: replace deprecated headers REF: meaningless static definition in anonymous namespace REF: init member in constructor Fix typo in variable name. Suppress issue in clang analyzer. CMake fixes. Remove unused CMake file. Implement ALLOW_CASE_INSENSIBILITY mode for StringToDoubleConverter c… Remove unnecessary INSTALL_INTERFACE expression. Use template for CMake installation. Fix mistake for build interface include dir. Improve CMake changes. Update CMake package generation. Implement ALLOW_CASE_INSENSIBILITY mode for StringToDoubleConverter class Add assert and test. Avoid negative shift. When filling in fractional digits in a fixed representation we might use all existing digits. When this happens, we can not look at the next digit to see if we should round up. Before this fix, we tried to shift by a negative amount to see if the (non-existing) next bit was set to 1 (requiring the number to be rounded up).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.