imglib / imglib2-algorithm

Image processing algorithms for ImgLib2
http://imglib2.net/
Other
22 stars 20 forks source link

ImgMath blockread and offset #88

Closed acardona closed 3 years ago

acardona commented 4 years ago

Additional features for ImgMath, including:

ctrueden commented 4 years ago

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

acardona commented 4 years ago

Ready to merge as far as I can tell.

acardona commented 4 years ago

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 commented 4 years ago

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 e93de982691923900d2ce332ba6e957996f004c2 breaks the build, and e8c7e43a7ce3f6092e7d15ab41a50d1bf1415c4c (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 commented 4 years ago

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 commented 4 years ago

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

acardona commented 4 years ago

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.