Skip to content

ImgMath blockread and offset #88

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

Merged
merged 36 commits into from
Sep 10, 2020
Merged

ImgMath blockread and offset #88

merged 36 commits into from
Sep 10, 2020

Conversation

acardona
Copy link
Contributor

@acardona acardona commented Aug 6, 2020

Additional features for ImgMath, including:

  • Ability to take a RandomAccessible as input, which enables e.g. offset reading, to create e.g. convolution filters or integral images (by reading from the prior pixel and writing onto the same image as the source).
  • Ability to do block reads by reading the values from 4 corners and combining them, namely for reading IntegralImg or any other form of integral image (also known as sum area tables) expressed as a RandomAccessible.
  • Ability to prune away null ops, such as adding zero, subtracting zero, dividing by 1, multiplying by 1, multiplying by 0, exponentiating (power) to 1, exponentiating a 1, and others. Simplifies operations which increases performance, and, critically, simplifying writing code to generate e.g. convolution filters.
  • Ability to take a KDTree plus a radius as input.
  • New operation minus, equivalent to sub(0, value), to add a minus sign to a value.
  • New operations log and exp, based on Math.log and Math.exp, not based on NumericType math because this type lacks these functions.
  • Small utility functions.

@acardona acardona force-pushed the ImgMath-blockread branch 2 times, most recently from 180ed00 to 35f0faa Compare August 6, 2020 14:35
acardona added 12 commits August 6, 2020 12:17
…Function

being the first place where the view methods are declared and defined.
some amount from it. It's like an ImgSource but for RandomAccessible
instead of for RandomAccessibleInterval. Only works with RandomAccess
and not with Cursor, so Compute had to adapt.
KDTreeSource reads from a KDtree<RealType>
BlockRead reads sums from an integral image.

Generalized with interface SourceInterval so that ImgSource
and KDTreeSource both return an Interval.

Util: new functions findFirstInterval, and wrap can now also handle
a RandomAccessible as a source.
@ctrueden ctrueden force-pushed the ImgMath-blockread branch from 35f0faa to 4215007 Compare August 6, 2020 17:17
@ctrueden
Copy link
Member

ctrueden commented Aug 6, 2020

I merged #87 and rebased this PR over top the newest master.

@acardona
Copy link
Contributor Author

Ready to merge as far as I can tell.

@acardona
Copy link
Contributor Author

Hi @ctrueden, are there any adjustments to the POM that should be made prior to merging, or can it be merged as is? Thanks!

@ctrueden
Copy link
Member

ctrueden commented Aug 30, 2020

are there any adjustments to the POM that should be made prior to merging

It doesn't look like you relied on any unreleased functionality from SNAPSHOTs, so probably it's all good.

Tangentially: you can use this script to check that all commits on the branch compile with passing tests. Here is the result on my system with the current branch:

check-branch.sh
0000 cea029bc7b26c4a83a57269567883c566ac24fe8 SUCCESS 17
0001 8449fd43cd912257baf88a099f9d6e0511877b30 SUCCESS 16
0002 7141e1a500f8b87d126408542edcf4f4bdfffe97 SUCCESS 16
0003 e4e418c86209d63794dceddb385d10929b9108a1 SUCCESS 18
0004 3b09f90ee23b48ea4e22ad15d8a517a118f98be9 SUCCESS 17
0005 3a6a827c727095577ede4822f46be3c9bb56b1f2 SUCCESS 17
0006 c81724e0851d79cedff1fbeab14a79b4e5a09656 SUCCESS 18
0007 37cf4f65aef4b365ef787f0a6ed2ec2d92fc476c SUCCESS 17
0008 ad6366e00c011d6bf0f7a9559b102739e1218c89 SUCCESS 16
0009 89efa4cd81ba3b196b6ee163113d2b00236a6312 SUCCESS 16
0010 579f4bde8c5abed9c96b20038c96efc2e6134bfb SUCCESS 18
0011 42150079d275d8dc58dc2c781c13189017a59ee3 SUCCESS 17
0012 b18f0c38a17fea353dda319892f807941335cc0c SUCCESS 16
0013 38b357f1a9a335064ec3dfb25f559348b8988cef SUCCESS 17
0014 d35ea53ebd225853e8c53f0a71bd406df576f22b SUCCESS 17
0015 7d2a021be1fcf6dc9d816cd5aacb0a8e8a48502c SUCCESS 16
0016 a8d758fcd86dfc83f837dca575879de4f86d64cf SUCCESS 17
0017 bbb78f6ccbc915042a059fd0e4a3ccaab47b01db SUCCESS 16
0018 e93de982691923900d2ce332ba6e957996f004c2 FAILURE 5
0019 45d0ebb3674da7878f122edff18b62c8769d06e5 FAILURE 5
0020 cd99c18f5beb6a5cbe70377391aec94f2a53e2a8 FAILURE 5
0021 64463855f3edda5a4ea89ec7730af517526c5ced FAILURE 5
0022 ed7cf2774837d0b23c4e6ea4c078924ff1040ea3 FAILURE 4
0023 e8c7e43a7ce3f6092e7d15ab41a50d1bf1415c4c SUCCESS 16
0024 3eb870fc5a63711b320169c6ddcfb7d3df750dc3 SUCCESS 16
0025 f45eb5cb1a889a505126ff6d1da55af38dcb792d SUCCESS 17
0026 b91252f6bfe5fa8bef0542a913634bb30d91c122 SUCCESS 17
0027 15a8e80d7184c417940edb1f616234e3a80e2122 SUCCESS 16
0028 a12b1e847f34ea03d722a4d5cd4387c93011262e SUCCESS 17
0029 a3d7d720cc90094fd426859ee64d93f2154a0c9c SUCCESS 18
0030 451503afbd84ad459ccf92c93bcffc1b7430e464 SUCCESS 17

So e93de98 breaks the build, and e8c7e43 (five commits later) fixes it. This will hamper use of git bisect inside this range later for isolating bugs.

Aside from that, one big problem I notice is that there are no unit tests for these new functions. It's really important to add some before merging.

I'm not a primary maintainer of this component—that's @tpietzsch and @axtimwalde and @StephanPreibisch. One of them should review and merge this work, not you or me.

@acardona
Copy link
Contributor Author

Thanks @ctrueden, regarding the 5 commits that fail, I'm happy to merge all those 5 commits into a single one, but it seems that would only make matters worse. Each change is small in any case, which is why I committed them separately for documentation.

Regarding tests: I understand automatic testing is desirable. I do all my testing in scripts, as this library is meant to be used from scripts, to enable bypassing performance limitations of low-level code in some scripting languages.

If writing tests from java as JUnit tests in this very same repository is a requirement, I'll try to find time for that eventually. They weren't there before for this library.

@acardona
Copy link
Contributor Author

acardona commented Sep 6, 2020

I have now added tests for all new classes as requested.

@acardona
Copy link
Contributor Author

acardona commented Sep 6, 2020

Given that the creation of ImgMath sublibrary was approved and this pull request merely adds additional classes in the same tune, without changing anything else outside of the net.imglib2.algorithm.math package, I will go ahead and merge this pull request in a few days unless objections are raised.

@acardona acardona merged commit 2fe0ce8 into master Sep 10, 2020
@acardona acardona deleted the ImgMath-blockread branch September 10, 2020 21:48
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