9il commented 7 years ago

Hi @ljubobratovicrelja! This is update for new ndslice. But this is only half of work. Please update examples and check that the new code works correctly. ~~Do not care about performance for now. I will update ndslice with @fastmath.~~(done)

9il commented 7 years ago

Possible issues: bad coverage and wrong SliceKind

9il commented 7 years ago

The docs are deprecated, ping me if you have any questions.

ljubobratovicrelja commented 7 years ago

This is great stuff, thanks Ilya! :)

Please update examples and check that the new code works correctly.

Gladly! Although I'll need some time to figure out new tools' API. For instance, when compiling features example, I got errors from dcv.imgproc.color:

../../source/dcv/imgproc/color.d(475,26): Error: no property 'gray' for type 'RefTuple!(Ref!(float[3]), Ref!float)'
../../source/dcv/imgproc/color.d(476,9): Error: no property 'gray' for type 'RefTuple!(Ref!(float[3]), Ref!float)'
../../source/dcv/imgproc/color.d(149,20): Error: template instance dcv.imgproc.color.rgb2grayImplMean!(RefTuple!(Ref!(float[3]), Ref!float)) error instantiating
../../source/dcv/imgproc/color.d(78,12):        instantiated from here: rgbbgr2gray!(false, float)
source/app.d(32,25):        instantiated from here: rgb2gray!float
../../source/dcv/imgproc/color.d(481,26): Error: no property 'gray' for type 'RefTuple!(Ref!(float[3]), Ref!float)'
../../source/dcv/imgproc/color.d(482,9): Error: no property 'gray' for type 'RefTuple!(Ref!(float[3]), Ref!float)'
../../source/dcv/imgproc/color.d(154,24): Error: template instance dcv.imgproc.color.rgb2grayImplLuminance!(RefTuple!(Ref!(float[3]), Ref!float)) error instantiating
../../source/dcv/imgproc/color.d(78,12):        instantiated from here: rgbbgr2gray!(false, float)

I need to figure out new zip and API of iterators.

Great job, I'm thrilled to see this coming to DCV so soon! :)

9il commented 7 years ago

Gladly! Although I'll need some time to figure out new tools' API. For instance, when compiling features example, I got errors from dcv.imgproc.color:

zip and RefTuple always have names a, b, c, ... . User can not set his own names like in Phobos. This reduce template bloat, API bloat, and compilation time. Probably rgb2gray is not covered by unittests in DCV

9il commented 7 years ago

BTW, this PR get back DMD support

9il commented 7 years ago

issues with colors fixed in https://github.com/libmir/dcv/pull/99/commits/0a344947c3963c1d9635bb492ff7de42bcbb614a . We can fork std.experimental.color and add @fastmath here. This will reduce color.d multiple times.

ljubobratovicrelja commented 7 years ago

@9il I see there is no indexSlice in mir-algorithm. Do we have an alternative?

ljubobratovicrelja commented 7 years ago

Do we have an alternative?

Found ndiota. Sorry for bothering.

9il commented 7 years ago

both dub test and dub test dcv:core pass now

9il commented 7 years ago

Good news: compilation + linking speed is 50% faster:

/// old
time dub build --compiler=ldmd2 --build=release-nobounds --skip-registry=all
real    0m17.070s
user    0m16.394s
sys 0m0.566s
time dub build --compiler=ldmd2 --build=plain --skip-registry=all
real    0m14.367s
user    0m13.638s
sys 0m0.569s

/// new
time dub build --compiler=ldmd2 --build=release-nobounds --skip-registry=all
real    0m11.107s
user    0m10.536s
sys 0m0.481s
time dub build --compiler=ldmd2 --build=plain --skip-registry=all
real    0m8.802s
user    0m8.224s
sys 0m0.479s

Note: To perform test run a command twice. After the first time remove .dub temporary folder

ljubobratovicrelja commented 7 years ago

@9il how do I extract SliceKind from a template type thats supposed to be Slice? Same as DeepElementType gets the type, I'd like to get the kind of slice.

9il commented 7 years ago

@9il how do I extract SliceKind from a template type thats supposed to be Slice? Same as DeepElementType gets the type, I'd like to get the kind of slice.

mir.ndslice.slice: kindOf in v0.0.12, needs few mins before code.dlang update

9il commented 7 years ago

Hi @ljubobratovicrelja . I have update mir-algorithm version to 0.0.14. It fixes slicing bugs.

9il commented 7 years ago

Mir GLAS passes all tests with the new ndslice

ljubobratovicrelja commented 7 years ago

Hey @9il, awesome news. Unfortunately, I've been super busy these days, having almost zero spare time to devote to dcv. Hopefully in a month or so, I'll be back.

ljubobratovicrelja commented 7 years ago

Hey @9il, been working on this and having some strange linking issues. Here is the dub compile output:

[~/P/dcv]─[⎇ new_ndslice]─[±]─> dub test --compiler=ldc2
Generating test runner configuration '__test__default__' for 'default' (library).
Performing "unittest" build using ldc2 for x86_64.
mir-internal 0.0.2: target for configuration "library" is up to date.
mir-algorithm 0.0.20: target for configuration "library" is up to date.
mir-math 0.0.1: target for configuration "library" is up to date.
dcv:core 0.1.7+commit.30.gb0faa0d: building configuration "library"...
Slice!(cast(SliceKind)2, [1LU], float*)
imageformats 6.1.0: target for configuration "library" is up to date.
dcv:io 0.1.7+commit.30.gb0faa0d: building configuration "library"...
dcv:plot 0.1.7+commit.30.gb0faa0d: building configuration "default"...
dcv 0.1.7+commit.30.gb0faa0d: building configuration "__test__default__"...
source/dcv/io/image.d(42,21): Deprecation: imageformats.std is not visible from module image
source/dcv/io/image.d(43,22): Deprecation: imageformats.std is not visible from module image
Undefined symbols for architecture x86_64:
  "__D3mir7ndslice8iterator65__T17FlattenedIteratorVE3mir7ndslice5slice9SliceKindi0VAmA1i2TPkZ17FlattenedIterator13__T8opEqualsZ8opEqualsMxFNaNbNiNfKxS3mir7ndslice8iterator65__T17FlattenedIteratorVE3mir7ndslice5slice9SliceKindi0VAmA1i2TPkZ17FlattenedIteratorZb", referenced from:
      __D3mir7ndslice8iterator65__T17FlattenedIteratorVE3mir7ndslice5slice9SliceKindi0VAmA1i2TPkZ17FlattenedIterator11__xopEqualsFKxS3mir7ndslice8iterator65__T17FlattenedIteratorVE3mir7ndslice5slice9SliceKindi0VAmA1i2TPkZ17FlattenedIteratorKxS3mir7ndslice8iterator65__T17FlattenedIteratorVE3mir7ndslice5slice9SliceKindi0VAmA1i2TPkZ17FlattenedIteratorZb in __test__default__.o

What do you make of it - Some mistake on my side or is there something in the mir-algorithm?

9il commented 7 years ago

This is D bug. Lets just find a workaround. Have you tried the last mir-algorithm version?

ljubobratovicrelja commented 7 years ago

Have you tried the last mir-algorithm version?

Yes, this is with v0.0.20.

9il commented 7 years ago

Please push all changes, I will try myself

ljubobratovicrelja commented 7 years ago

Please push all changes

Done.

Features example does not work right for some reason, I'll investigate that in future. I think it has something to do with the extractCorners. Besides that, filters example still doesn't compile, but all other examples compile and run properly.

ljubobratovicrelja commented 7 years ago

Fixed filter example so it compiles, but running bilateralFilter causes:

fish: './filter' terminated by signal SIGFPE (Floating point exception)

Here is the lldb break:

Process 9489 launched: '/Users/relja/Projects/dcv/examples/filter/filter' (x86_64)
Process 9489 stopped
* thread #1: tid = 0x4457e, 0x0000000100083c36 filter`D3mir7ndslice5field21__T11ndIotaFieldVmi2Z11ndIotaField12__T7opIndexZ7opIndexMxFNaNbNiNfmZG2m + 90 at field.d:147, queue = 'com.apple.main-thread', stop reason = EXC_ARITHMETIC (code=EXC_I386_DIV, subcode=0x0)
    frame #0: 0x0000000100083c36 filter`D3mir7ndslice5field21__T11ndIotaFieldVmi2Z11ndIotaField12__T7opIndexZ7opIndexMxFNaNbNiNfmZG2m + 90 at field.d:147
   144          size_t[N] indexes = void;
   145          foreach_reverse (i; Iota!(N - 1))
   146          {
-> 147              indexes[i + 1] = index % _lengths[i];
   148              index /= _lengths[i];
   149          }
   150          indexes[0] = index;
  thread #5: tid = 0x445b0, 0x0000000100083c36 filter`D3mir7ndslice5field21__T11ndIotaFieldVmi2Z11ndIotaField12__T7opIndexZ7opIndexMxFNaNbNiNfmZG2m + 90 at field.d:147, stop reason = EXC_ARITHMETIC (code=EXC_I386_DIV, subcode=0x0)
    frame #0: 0x0000000100083c36 filter`D3mir7ndslice5field21__T11ndIotaFieldVmi2Z11ndIotaField12__T7opIndexZ7opIndexMxFNaNbNiNfmZG2m + 90 at field.d:147
   144          size_t[N] indexes = void;
   145          foreach_reverse (i; Iota!(N - 1))
   146          {
-> 147              indexes[i + 1] = index % _lengths[i];
   148              index /= _lengths[i];
   149          }
   150          indexes[0] = index;

@9il could you take a look at this one also? Bug seems to be tied to your entry here.

ljubobratovicrelja commented 7 years ago

Interestingly, filter example is failing with ldc (take a look at travis report), but it compiles with dmd.

ljubobratovicrelja commented 7 years ago

@9il I've reverted corner drawing in 09ec8840ff16bae2d95747ea243551fede848df5, new one was causing issues, not completely sure why. Anyhow, let's not worry about having to map values to extract x's and y's there - we'll surely change this api to slices, after which dimension selection will be trivial.

9il commented 7 years ago

Hey @ljubobratovicrelja ! Sorry for the delay with the issue

The bug was caused by an unittest in slice.d. It is DUB or DMD FE bug. I done workaround.

Try to do the following:

update your local repo with my latest commit
remove dub.selections.json
dub test --compiler=ldmd2 --force

It works for me with the latest mir-algorithm v0.0.22.

ljubobratovicrelja commented 7 years ago

It works for me with the latest mir-algorithm v0.0.22.

Awesome, tnx! I've fixed remaining issue in tests, now travis is giving us a green light! But there's still the issue with filtering example and bilateral filter. I'll try looking over this in the next few days. That said, is it a good idea to have travis dub run our examples instead just build? We'd have to modify examples a bit, but I think we should do this. Thoughts?

Once all examples are up and running, besides documentation fixup, is there anything else left to do before merge?

9il commented 7 years ago

That said, is it a good idea to have travis dub run our examples instead just build? We'd have to modify examples a bit, but I think we should do this. Thoughts?

Sounds good

Once all examples are up and running, besides documentation fixup, is there anything else left to do before merge?

No, a forum announce after would be good

ljubobratovicrelja commented 7 years ago

a forum announce after would be good

👍

I'll try fixing up bilateralFilter, parse trough the docs, merge, release, and after that we should make the announcement. If you wish to take over any of these tasks, let me know.

codecov-io commented 7 years ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@030a71b). Click here to learn what that means. The diff coverage is 80.19%.

@@            Coverage Diff            @@
##             master      #99   +/-   ##
=========================================
  Coverage          ?   70.88%           
=========================================
  Files             ?       25           
  Lines             ?     2507           
  Branches          ?        0           
=========================================
  Hits              ?     1777           
  Misses            ?      730           
  Partials          ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 030a71b...edbbe07. Read the comment docs.

ljubobratovicrelja commented 7 years ago

@9il bilateralFilter seems to be fixed with 78b43191351bed515a0fc91c3fecafd7f78405c8 (the result looks correct). But could you take a look at the diff, and see everything is alright? I'm not completely sure I've used ndIotaField the right way.

Also, hope you don't mind, I've fixed a bug non-related to this PR, long standing in ggplot-d drawing (it was causing crash in lucas-kanade example). Now every example seems to be running perfectly on my machine.

ljubobratovicrelja commented 7 years ago

@9il I did all I intended to. Good to merge?

ljubobratovicrelja commented 7 years ago

Forgot to mention - there's a little bit more work regarding examples running in CI tests than I expected. I'd like to take care of this some other time, hope you're ok with that.

9il commented 7 years ago

@ljubobratovicrelja You may have similar issues with LDC, but DMD should work fine. Hope to see new LDC release soon

ljubobratovicrelja commented 7 years ago

You may have similar issues with LDC, but DMD should work fine

To what are you referring? libmir/mir-algorithm#19?

9il commented 7 years ago

Yes

ljubobratovicrelja commented 7 years ago

@9il I did all I intended to. Good to merge?

ping

9il commented 7 years ago

Yes, please merge

libmir / dcv

New ndslice #99

Codecov Report