Skip to content
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

IdentityOperator should take a range argument #1069

Open
kohr-h opened this issue Jul 9, 2017 · 8 comments
Open

IdentityOperator should take a range argument #1069

kohr-h opened this issue Jul 9, 2017 · 8 comments

Comments

@kohr-h
Copy link
Member

kohr-h commented Jul 9, 2017

We should allow the identity operator (and together with it ScalingOperator and a couple more) to map to a different space. For instance from rn(5) to uniform_discr(0, 1, 5).

@adler-j
Copy link
Member

adler-j commented Jul 9, 2017

Duplicate of #59?

@kohr-h
Copy link
Member Author

kohr-h commented Jul 9, 2017

Yes, to some degree. But the implementation here would be return self.range.element(x), thus only allowing the "safe" variant. Here it's more obvious from the name "identity" since a mapping with information loss would hardly qualify as identity.

@kohr-h
Copy link
Member Author

kohr-h commented Jul 9, 2017

Or, as opposed to the implementation in #95, we wouldn't have to worry about mappings from, say, L1 to L2 since "identity" is also mathematically valid, while "embedding" isn't.

@adler-j
Copy link
Member

adler-j commented Jul 9, 2017

Yes, to some degree. But the implementation here would be return self.range.element(x), thus only allowing the "safe" variant.

Well, the adjoint would not be as simple so its not trivial to implement. Anyway I think that issue had some work on this as well, as you mention about #95.

@adler-j
Copy link
Member

adler-j commented Jul 9, 2017

Anyway my point is that I'm not at all sure I want to add this to Identity since it would need lots of other code to be added (adjoint stuff, for example) that would cause weird dependencies.

I'd probably prefer a new class for this, or some other construction anyway.

@kohr-h
Copy link
Member Author

kohr-h commented Jul 10, 2017

since it would need lots of other code to be added

That's true, but I think that part will be very widely applicable and can be factored out to some helper function. When you have an operator A: X_v -> Y_w with weights v and w, and the same operator B: X -> Y between unweighted spaces, you always get this:

<Af, g>_(Y_w) = <Bf, wg>_Y = <f, B*(wg)>_X = <f, v^(-1)B^*(wg)>_(X_v)

Then you can move all the logic to a function that can also distinguish cases, e.g., merge weighting factors when one of them is a constant, or ignore constant 1.0 etc. Then operators just need to use that function, and there's no weird dependency whatsoever.

@adler-j
Copy link
Member

adler-j commented Jul 10, 2017

Well, currently the whole default_ops package does not know about any specific spaces as all (I guess thats why they are "default"), i.e. they only know about LinearSpace. Changing this would be somewhat drastical and should be motivated.

You could make a PR (most important code will be copy-pastable anyway) and we'll see how it looks. The functionality is needed regardless so this is only a question of where we want it.

@kohr-h
Copy link
Member Author

kohr-h commented Jul 10, 2017

Well, currently the whole default_ops package does not know about any specific spaces as all (I guess thats why they are "default"), i.e. they only know about LinearSpace. Changing this would be somewhat drastical and should be motivated.

True, but what we call default_ops is more like default_ops_on_default_spaces or so. It's simply not possible to define a correct adjoint without looking at the spaces.
But it would be a significant change and requires some more thinking.

You could make a PR (most important code will be copy-pastable anyway) and we'll see how it looks. The functionality is needed regardless so this is only a question of where we want it.

I'll see what I can do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants