Issue 59 coerce #95

adler-j · 2015-11-26T16:49:00Z

Initial attempt at issue #59. Not ready for pull.

Some points of discussion/additions

Verify DiscreteLp <-> DiscreteLp especially if they have different data orders.
Look into optimizations (should be able to be zero overhead in some situations).
I was forced to add a method FnBaseVector.asflatarray. I highly dislike this, but it shows a problem we have in DiscreteLpVector.asarray, namely x.asarray()[index] != x[index], imo this doesnt make much sense. I would suggest we move the current behaviour to a method called DiscreteLpVector.asvoxels() (or preferably some better name), and leave the asarray behaviour to match that of Discretization.

Edit: Also solved issue #83 since it was "needed" for this.

kohr-h · 2015-11-26T22:23:03Z

odl/operator/embedding.py

+
+class Embedding(Operator):
+    """An operators to cast between spaces."""
+    def __init__(self, target, origin):


Why do you switch order here? IMO it makes much more sense to mimic Operator.

This has plagued me. Basically it "feels good" to write (if op has Rn as domain)

op * embed(rn, cudarn)

This is because you can read it nicely from the right. In the end I'll have to agree however, keeping things "as is" is probably better than having a somewhat more convenient special case here.

I would always read this as "embed rn into cudarn", but I'm spoiled by math :-)

And the "feel bad" vs "feel good" is the same as in math, where you have to think twice when your read A o B and need to map domains and ranges. I can see your point, reading as a "feed" from right to left is somewhat charming, but it interferes too much with the "math common sense".

Will get this fixed.

kohr-h · 2015-11-26T22:49:07Z

was forced to add a method FnBaseVector.asflatarray. I highly dislike this, but it shows a problem we have in DiscreteLpVector.asarray, namely x.asarray()[index] != x[index], imo this doesnt make much sense. I would suggest we move the current behaviour to a method called DiscreteLpVector.asvoxels() (or preferably some better name), and leave the asarray behaviour to match that of Discretization.

I see the problem. The proper solution to this would be multi-indexing, but that's a large step to take on the CUDA side for such a relatively small fix. The second best solution is what you did. We can for sure adapt the names, maybe asmultiarray() would fit the picture. But in the long run, I think we should keep the pain to switch to multi-indexing in DiscretizationVector by default as painless as possible.

kohr-h · 2015-11-26T22:52:49Z

Verify DiscreteLp <-> DiscreteLp especially if they have different data orders.

Probably some options needed, definitely (imo) strict=True which would disallow embedding Lp into Lq when p > q. For the math nazis.
The reordering issue could be controlled with a allow_reorder=False switch.

adler-j · 2015-11-26T22:57:02Z

Multi-indexing would also require any further implementations to provide full slicing, I dislike that.(what if a user wanted to use ASTRA storage, would it need slicing?). We'll have to think abit more about this.

kohr-h · 2015-11-26T23:15:04Z

It's not a super pressing issue anyway. Linear arrays are good since anybody supports them, while multiarrays are fancy and in 90 % of the cases require flattening to interact with other libraries. Let's postpone that.

kohr-h · 2015-12-01T13:11:10Z

odl/operator/embedding.py

+    def __init__(self, origin, target):
+        assert isinstance(origin, DiscreteLp)
+        assert isinstance(target, DiscreteLp)
+        assert origin.grid.shape == target.grid.shape


This should only be possible if target.exponent <= origin.exponent for mathematical reasons.

Not sure about that, embedding (to me) means, "Do as your told and ignore the consequences". Kindoff like how mathematicians often "identify" things with others. Perhaps the vectors given to this will only be in a subspace where it still works? I think we need a way for users to "force" this through.

This guy could take a parameter strict=True which would check the mathematical condition. To me, an embedding is the identity mapping from a space to a superspace, not simply closing your eyes. If users want to go for it, they can set strict=False. We can also call it force=False.

This guy could take a parameter strict=True which would check the mathematical condition.

Sure, but I'd default it to False.

To me, an embedding is the identity mapping from a space to a superspace, not simply closing your eyes.

Then this needs to be renamed. Very few of these are actually to superspaces, and the adjoint would then never be defined (which obviously isn't that useful).

Then this needs to be renamed. Very few of these are actually to superspaces, and the adjoint would then never be defined (which obviously isn't that useful).

It's a projection then. In that case I'm good with not checking the exponent.

kohr-h · 2015-12-01T15:29:49Z

Well, for now it seems like the only way of getting away with it. I don't think it's so bad. There's still the alternative suggestion to make flat default and call the other one asmultiarray.

adler-j · 2015-12-01T15:34:51Z

How about asndarray? Kinda double meaning, both tells you nd and tells you its a ndarray.

adler-j · 2015-12-06T15:06:12Z

Ok, things left to consider here:

Naming of asflatarray, is it good?
Adjoint handling (it should be correct)
Add inverse
Naming, should we change Embedding to something else?

kohr-h · 2015-12-06T16:23:33Z

Naming of asflatarray, is it good?

I think it's actually better this way, that we internally have to use the less intuitive (asflatarray) method while users can go with asarray() and get a nice reshaped multiarray.

Naming, should we change Embedding to something else?

Mathematically, the functionality defines a projection. Embedding is not correct, and I don't think we should callit like that then.
Projection unfortunately collides with the notion of projection in tomography, and we should avoid that. Maybe ProjectionMapping?

adler-j · 2015-12-07T10:53:11Z

I think it's actually better this way, that we internally have to use the less intuitive (asflatarray) method while users can go with asarray() and get a nice reshaped multiarray.

OK, But I think numpy has some of these around, I'll see if they have a naming convention.

Mathematically, the functionality defines a projection. Embedding is not correct, and I don't think we should callit like that then.
Projection unfortunately collides with the notion of projection in tomography, and we should avoid that. Maybe ProjectionMapping?

You are correct but I don't quite like the sound of that (a bit too long when you start adding cases). I'll think about it some more.

adler-j · 2015-12-28T10:56:40Z

This has to be taken up at some point, Ill see if I can get to it. Meanwhile regarding asflatarray, numpy has a reference in nonzero and flatnonzero:

https://github.com/numpy/numpy/blob/183fdb290cd46b1f01303d24ac0c9fc3ff24fe05/numpy/core/numeric.py#L791

adler-j · 2016-11-02T23:02:45Z

Should we close this for now? I guess this is so old a new effort would be needed if we want it. I wont delete the branch, for now.

kohr-h · 2016-11-03T09:31:33Z

There doesn't seem to be an urgent need for it right now, so I guess we can have the code lying around in this branch and close the issue for now, as you suggested.

adler-j added 3 commits November 26, 2015 16:40

ENH: add powerspace.asarray(), see #83

38f1a35

MAINT: pep8 and build fix

f8c556d

ENH: add embedding operators

0650afa

kohr-h reviewed Nov 26, 2015
View reviewed changes

adler-j added 2 commits December 1, 2015 10:20

ENH: changed order of arguments in Embedding

726475c

ENH: update import order in odl/__init__.py to match dependencies

a285d29

kohr-h reviewed Dec 1, 2015
View reviewed changes

adler-j added the type: deficiency label Jan 14, 2016

adler-j closed this Nov 3, 2016

adler-j deleted the issue-59__coerce branch January 18, 2017 21:06

kohr-h mentioned this pull request Jul 9, 2017

IdentityOperator should take a range argument #1069

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 59 coerce #95

Issue 59 coerce #95

adler-j commented Nov 26, 2015

kohr-h Nov 26, 2015

adler-j Nov 26, 2015

kohr-h Nov 26, 2015

kohr-h Nov 26, 2015

adler-j Nov 26, 2015

kohr-h commented Nov 26, 2015

kohr-h commented Nov 26, 2015

adler-j commented Nov 26, 2015

kohr-h commented Nov 26, 2015

kohr-h Dec 1, 2015

adler-j Dec 1, 2015

kohr-h Dec 1, 2015

adler-j Dec 1, 2015

kohr-h Dec 1, 2015

kohr-h commented Dec 1, 2015

adler-j commented Dec 1, 2015

adler-j commented Dec 6, 2015

kohr-h commented Dec 6, 2015

adler-j commented Dec 7, 2015

adler-j commented Dec 28, 2015

adler-j commented Nov 2, 2016

kohr-h commented Nov 3, 2016

Issue 59 coerce #95

Issue 59 coerce #95

Conversation

adler-j commented Nov 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kohr-h commented Nov 26, 2015

kohr-h commented Nov 26, 2015

adler-j commented Nov 26, 2015

kohr-h commented Nov 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kohr-h commented Dec 1, 2015

adler-j commented Dec 1, 2015

adler-j commented Dec 6, 2015

kohr-h commented Dec 6, 2015

adler-j commented Dec 7, 2015

adler-j commented Dec 28, 2015

adler-j commented Nov 2, 2016

kohr-h commented Nov 3, 2016