-
Notifications
You must be signed in to change notification settings - Fork 93
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
Projectors #23
Projectors #23
Conversation
+ a bit cleaning up
also changed the arithmetic but the results should be equal
The old XYProjector and XYRandomAccessibleProjector should be unified with the new projector hierarchy. CompositeProjector and DimProjector should be unified. It would be really nice if we could use this as an opportunity to clean up the display package instead of convoluting it even further. |
added a benchmark XYProjector vs Projector2D
I tested Projector2D X=0 Y=1 vs XYProjector on some random images. On my machine they are equally fast |
any further comments? what needs to be done before merging? |
As I said before, it would be nice if the display package could be cleaned up. This would involve making XYProjector and XYRandomAccessibleProjector extend Projector2D and RandomAccessibleProjector2D (doesn't exist yet). Find a unifying solution for CompositeXYProjector, DimProjector2D maybe involving Saalfelds new Composite stuff (I had hoped that Stephan would comment here, but I guess thats not going to happen). Move the more general Converters to the net.imglib.converter package (or a subpackage) because they are not display specific. Maybe get rid of the unused generic parameters in the Projector interface. I cannot provide detailed step-by-step instructions on what I would like to see done exactly, sorry. In the time that would require me to figure out and describe , I could also do it myself. I'm not using the projectors at the moment and anyway I think the display package is a bit of a mess right now. So actually you could also just merge now and put this additional stuff in, it will not get worse by that. |
Thanks for your comments, @tpietzsch. I agree that what is needed here is just another iteration of development according to the suggestions you have given, followed by another code review. It shouldn't be necessary to offer exact step-by-step instructions as these things sort of emerge on their own as you are coding. So anyway I think it would be worth it for @MichaelZinsmaier to take another pass at accomplishing some of these goals, as long as it is not too time consuming. Note that GitHub has some great code review features: you can comment on PRs like we are doing now, but also on specific commits of a PR (or any commit, really), or even specific lines of a commit. So @tpietzsch, if you see specific places in the PR that you want to point out, you can do this. |
Sorry. I have commented on that in a different context but not executed the intended performance tests because I ran out of time due to other issues. Currently, I cannot do anything either. Regarding composites, my draft is available in two interactive imglib2-examples. They demonstrate how you can collapse a dimension into a Composite (check Views.collapse), and then use the basic Projector concept with a Converter to convert from a Composite to an ARGBType. My thinking is that this is a more elegant solution than special purpose CompositeProjectors but I haven't had the time yet to investigate potential performance penalties, nor is the design set in stone. Tobias has pointed out rightly that carefully reading, commenting, and discussing someone else's code can become similar or higher effort than writing it from scratch (even with GitHub's great annotation features). And, sadly, I currently do not have the time to review the display package. To not stop progress, I suggest to merge and expect it to be revised and broken any time in the future. Sorry, I know that this isn't very satisfying. |
@axtimwalde: I agree that detailed review, especially of large patches, can take quite some time. And while code review improves the quality of the design, I think there is a balance. In this case I think @tpietzsch struck it right: he recommended some focused directions without getting into nitpicks of specific lines of code, so that @MichaelZinsmaier and @dietzc can do another design iteration. Personally I believe that faster iteration is superior to slower "high-quality" iteration.
I think it is OK as long as ImgLib2 is still in beta. There will come a day when we need to commit to the API, though. (And even then, adding new API and deprecating old API is fine; we just have to be more careful about removing things and changing behavior.) |
We started the clean-up. Some comments:
If you have any further comments, suggestions or anything else, we would be happy to further improve the package structure. Afterwards we would suggest to merge :-) Cheers, |
@tpietzsch: Thanks for the suggestions.
|
@dietzc: Just a reminder that apart from the |
Conflicts were nearly all import skew due to removal of imglib2-io. Conflicts: core/src/main/java/net/imglib2/display/VolatileXYRandomAccessibleProjector.java examples/src/main/java/interactive/Interactive3DRealRandomAccessibleExample.java examples/src/main/java/interactive/MandelbrotRealViewer2DExample.java tests/src/test/java/tests/LanczosExample.java tests/src/test/java/tests/OpenAndDisplayAffineTransformedScreenImage.java
I just noticed that the new Projector2D is 25% slower on real-transformed images than the old XYRandomAccessibleProjector. Unfortunately XYRandomAccessibleProjector was removed by this PR without a proper replacement. I think this PR was merged prematurely and maybe a bit carelessly (for example, how did this happen? : https://github.com/imglib/imglib-tests/blob/master/src/test/java/tests/XYRandomAccessibleProjector.java) I would like to reinstate the old XYRandomAccessibleProjector until somebody has time to make a proper RandomAccessibleProjector2D? What do you think @dietzc, @MichaelZinsmaier, @ctrueden? |
Hi Tobias, |
why to wait until tomorrow: I just created a new branch with a few commits (https://github.com/imglib/imglib/tree/p2d-improvements). I added a new RandomAccessibleIntervalProjector2D and renamed Projetor2D to IterableIntervalProjector2D. These implemenations are not benchmarked and not tested yet. I can/will do this tomorrow. if you want me to benchmark certain cases, you can send me your benchmark. |
Benchmarks that are made part of the unit tests, in particular when they mark performance regressions as failures, would have caught this regression. Maybe it would be a good idea to prevent the same thing from happening by adding proper benchmark tests. |
@dietzc I will put a benchmark on the p2d-improvements branch and let you know. I played around a bit yesterday with the projector used in ImageJVirtualStack (I was using that and it seemed slow which is why I started looking into this). It turned out that also when just projecting from an ArrayImg XYRandomAccessibleProjector only took 2/3 of the time Projector2D took. This seems a bit unintuitive, so maybe it's caused by something else that confused the JIT. @dscho True. I just saw your comment imagej/imagej-ops#6 on junit-benchmarks. I didn't know about junit-benchmarks, I'll have a look! |
@dietzc I put a benchmark in the p2d-improvements branch: https://github.com/imglib/imglib/blob/ea6992e0e51d52f323214e904715020472c21629/realtransform/src/test/java/net/imglib2/display/ProjectorBenchmark.java Results for me are: IterableIntervalProjector2D Img When running all tests in sequence (causing JIT deoptimization), median / best times over 100 runs converge to IterableIntervalProjector2D Img Your RandomAccessibleProjector2D is almost as fast as XYRandomAccessibleProjector. |
@tpietzsch I found one more thing to optimize in the IterableIntervalCursor. We used localizingCursors even if we didn't need them. I will commit, run benchmark and post them here. Give me 30min! and thanks for the bug-fix. didn't test it yet ;-) |
@tpietzsch i pushed the modifications on IterableIntervalProjector2D. Here are my benchmarks when each test is run individually (100times): IterableIntervalProjector2D Img RandomAccessibleProjector2D Img XYRandomAccessibleProjector Img IterableIntervalProjector2D Affine RandomAccessibleProjector2D Affine XYRandomAccessibleProjector Affine Sequence: IterableIntervalProjector2D Affine RandomAccessibleProjector2D Img RandomAccessibleProjector2D Affine XYRandomAccessibleProjector Img XYRandomAccessibleProjector Affine can you confirm these results? what do you think? @ctrueden while actually hacking on this, I realized, that this is also a very nice case where we could use scijava-commons: If there exists a very fast implementation to render this kind of image (e.g. Plane-Wise ArrayImg of type ByteType, see ArrayImgXYByteProjector) then use this projector. Pretty much reminds me of imagej-ops, doesn't it? |
@dietzc I can confirm your results. Very nice! Thanks a lot! @ctrueden @dietzc Wrt. to scijava-commons, it would be educational to see how well this would work in this case. If one of you finds time to give it a try, I would be very interested. |
As pointed out multiple times over the past years: median is not the correct measure here, unless you really want to benchmark the run time of the algorithm together with delays caused by your mail client, your web browser, your disk cache, etc. |
imglib-tests and imglib-tutorials live in their own repositories now, as agreed upon some month ago. They should depend on release versions of imglib so that we get closer to reproducible builds that @axtimwalde asked for all these years ago. |
Okay, I was stupid and indeed dropped everything I really have to do in favor of fixing the imglib-tests and imglib-tutorials projects. But they are already fixed! They already depend on (beta) release versions of imglib! So @tpietzsch what am I missing??? |
@tpietzsch that does not explain why you had a problem with imglib-tests and imglib-tutorials. |
@tpietzsch I didn't have tests in my eclipse workspace, so eclipse unfortunately didn't do the job. Anyway: I don't see any compilation errors (just freshly imported tests as maven project to do the refactoring). Something is strange ;-)) |
@dscho @dietzc I have locally modified the imglib-tests and imglib-tutorials projects to use the 2.0.0-SNAPSHOT version of imglib. That way, refactorings in one project carry over to the others automatically. I think it makes a lot of sense to do this. Otherwise the next poor soul who wants to bring those projects "up to speed" will need to dig through the git history to see what happened. Why do that, if you can have Eclipse do it right away... Probably it would be good to establish some kind of best pratice to do this, e.g., identically named branches on the other projects. What do you think? Sorry for not being clear about what I meant earlier. There are no compile errors with imglib-tests and imglib-tutorials depending on the imglib beta. There will be problems when they are bumped to the next beta version that will include @dietzc's refactorings, and it would make that transition smoother by having the corresponding refactorings in branches in imglib-tests and imglib-tutorials already. Sorry about the confusion. |
@tpietzsch okay, you can do that. But most likely others did not because -SNAPSHOT bindings bring with them a whole world of pain, as @axtimwalde explained patiently at the EuroBioImaging meeting in Barcelona 2012. So I doubt that @dietzc has done that (because it would distract him from the real goal). Therefore I think you'll have to fix that breakage yourself, or go back to release bindings. |
Okay, I can go back to release bindings then. My intention was just to make the transition process for the next releases of imglib-tests and imglib-tutorials as smooth as possible. But of course we can also fix that later. We all have too much time on our hands anyway. |
I'm a little confused at the moment, about what we actually have or want. It's not about what I want to do, I simply don't know which option is correct? Option (b) makes sense. @dscho, @tpietzsch, @ctrueden, @axtimwalde any suggestions? (keep it simple as the problem is simple ;-)). |
My vote is to worry about imglib2-tests et. al later. No one consumes this artifact, and it doesn't ship with Fiji; to put it bluntly, it isn't really important. (To a lesser extent, this is also true of imglib2-scripting.) We are moving away from multi-module releases versioned together, toward each component having its own version. This means that in the future, e.g. imglib2-tests may have a different number of releases than, and be at a different version number than, say, imglib2 core. This is OK, and means it becomes much simpler to make new releases of only a component that changed, rather than having to cut a release of the entire imglib2 stack when most of the components had no changes. The rule of thumb (which we may not follow 100% in all cases, but it is a general guideline) is that each .git repository has one or more artifacts which are versioned together. It is possible that ultimately the imglib.git itself may not even be a multi-module build anymore, if we get to the point where every component lives in its own git repository. I guess it will depend on how intimately tied together some of the components are with one another. The development pattern has been that during the early rapid development phase, we have more modules in a multi-module build a la imglib.git and imagej.git, and over time as things become more stable, pieces of those repositories are split out into separate git repositories which become versioned separately. Regarding imglib2-tests specifically: note that it contains only manual tests and examples, as opposed to unit tests. My preference would be for any critical functionality being tested by imglib2-tests to be converted into unit tests, so it gets tested automatically by Jenkins with every build. And for any "demo" or "example" type code in there to be converted into imglib2-tutorials and documented clearly for the community. If we did that in every case, we could actually get rid of imglib2-tests completely. Failing that, the remaining imglib2-tests could become "integration tests" which are handled by either maven-failsafe-plugin or maven-invoker-plugin, and run automatically by Jenkins, perhaps on a daily basis. But still in that case we should consider moving them into the most relevant other imglib component, rather than leaving them separate as they are now. If it helps I could write this stuff up on the SciJava wiki somewhere. The development process is still evolving as the SciJava stack becomes more mature, so it's not like we have all the answers in the bag. We're just trying to do what works and will continue to work as time goes on. If anyone disagrees with this direction, we can certainly discuss and hash out a better way. |
@dietzc Good idea. I filed #66 so we could pursue it later as time allows. |
A couple other things about the release bindings between components at different versions:
Eventually we will definitely produce a more formal write-up of these details, once this "QA" process is fully in place. |
KNIP Projetor and ScreenImage classes
see Issue #19