Skip to content

WIP real-valued functional #1333

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.

Sign up for GitHub

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

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

WIP real-valued functional #1333

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
d4e5498
Tried to make functional possible to map to real numbers istead of sp…
Apr 26, 2018
7a278f0
Merge branch 'master' of https://github.com/miallo/odl
Apr 26, 2018
09b1e94
Continued implementation of functionals mapping to RealNumbers
Apr 26, 2018
89652b1
Added range to functional operations
Apr 27, 2018
69521ba
Continued to implement range in default_functionals
Apr 27, 2018
85650f7
Mostly continued default_fuctionals
Apr 27, 2018
7382635
fixed mistake where renaming proximal_l* function inputs to `domain`
Apr 27, 2018
246a31b
fixed range of QuadraticForm-Functional
Apr 27, 2018
5a55e75
Implemented most comments from @kohr-h and @aringh
Apr 27, 2018
d1c4d68
Is there a reason for ufunc_ops to pass the explicit keyword `domain=…
Apr 27, 2018
9003572
Added docstrings for `range` parameter
Apr 27, 2018
77842df
Fields can now be added to the "highest" one
Apr 29, 2018
c7a6127
Removed addition of fields again, Reverted `__repr__` not to include …
Apr 29, 2018
3fa93d9
Changed the order to (domain, range, ...) in __init__ calls and fixed
May 31, 2018
0951041
fixed typo in forward-backward documentation
Jun 5, 2018
0ddd06c
Merged from odl main branch
Jun 13, 2018
cb6df08
CallbackShowConvergence now supports saveto
Jun 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions odl/operator/operator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1071,7 +1071,6 @@ class OperatorSum(Operator):

The sum is only well-defined for `Operator` instances where
`Operator.range` is a `LinearSpace`.

"""

def __init__(self, left, right, tmp_ran=None, tmp_dom=None):
Expand Down Expand Up @@ -1106,8 +1105,14 @@ def __init__(self, left, right, tmp_ran=None, tmp_dom=None):
rn(3).element([ 2., 4., 6.])
"""
if left.range != right.range:
raise OpTypeError('operator ranges {!r} and {!r} do not match'
''.format(left.range, right.range))
if isinstance(left.range, Field) and \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please no backslash line continuation. To split across lines, use parentheses:

if (isinstance(left.range, Field) and
        isinstance(right.range, Field)):
    # do stuff

isinstance(right.range, Field):
range = left.range + right.range
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just as an aside, you can make this a one-liner without implementing __add__:

range = max(left.range, right.range, key=lambda s: 0 if s == RealNumbers() else 1)

Slightly hackish since it assumes that else means ComplexNumbers, but it works 😉 .

else:
raise OpTypeError('operator ranges {!r} and {!r} do not match'
''.format(left.range, right.range))
else:
range = left.range
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I'd prefer not to have any clever mechanism for "fixing" cases that "obviously" should work, like

L1Norm(cn(2), range=ComplexNumbers()) + L1Norm(cn(2), range=RealNumbers())

IMO this should fail, and users should explicitly have to use RealPart or ComplexEmbedding to state their intentions (of course, for that, those operators need to work with RealNumbers and ComplexNumbers, resp.).

if not isinstance(left.range, (LinearSpace, Field)):
raise OpTypeError('`left.range` {!r} not a `LinearSpace` or '
'`Field` instance'.format(left.range))
Expand All @@ -1124,7 +1129,7 @@ def __init__(self, left, right, tmp_ran=None, tmp_dom=None):
''.format(tmp_dom, left.domain))

super(OperatorSum, self).__init__(
left.domain, left.range,
left.domain, range,
linear=left.is_linear and right.is_linear)
self.__left = left
self.__right = right
Expand Down
24 changes: 20 additions & 4 deletions odl/set/sets.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,25 @@ def field(self):
"""
return self

def contains_set(self, other):
raise NotImplementedError('field {!r} does not have method '
'`contains_set`'.format(self))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error would already be raised by the parent, wouldn't it?


def __add__(self, other):
if self.contains_set(other):
return self
elif other.contains_set(self):
return other
else:
raise ValueError('Fields {!r} and {!r} do not include '
'each other'.format(self, other))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm skeptical towards this. It doesn't fulfill the rules for set addition, and just as a shortcut for something used in OperatorSum, it shouldn't introduce an API change for Field.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am adding fields, not sets and so I thought I went with the Minkowski addition A + B = {a + b | a in A, b in B} (which at least for the implemented numbers is trivial). I actually haven't thought about other sets.

What would be your suggestion to approach this? At first I did something like if isinstance(a.range, ComplexNumbers) or isinstance(b.range, ComplexNumbers): return ComplexNumbers(); elif ... but that was rather unpleasant to look at...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am adding fields, not sets and so I thought I went with the Minkowski addition A + B = {a + b | a in A, b in B} (which at least for the implemented numbers is trivial). I actually haven't thought about other sets.

I was thinking about that definition as well. My point is that IMO, it makes little sense to implement an operation that would be valid (but hard to implement in general) for any Set only for a subclass. So anything short of extending Set, or at least Field, in a generic way is not really desirable. Sure, if all we are dealing with are either real or complex numbers, adding them is trivial. But what if someone decides to add a finite field class -- adding them might not even produce a new field, depending on the definition of addition.

The main issue is that the means you chose are way too generic and sweeping for the problem you're trying to solve. Assuming that this problem actually needs to be solved (which it might not, see my other comments), the least invasive solution would be to introduce an ordering of the sets you're looking at and then just use the max function:

# At module level
_SET_ORDER = {Integers(): 1, RealNumbers(): 2, ComplexNumbers(): 3}

# Somewhere else
field = max(field1, field2, key=lambda f: _SET_ORDER[f])

# Or making a function (only for internal use)
def _largest_field(*fields):
    _SET_ORDER = {Integers(): 1, RealNumbers(): 2, ComplexNumbers(): 3}
    return max(fields, key=lambda f: _SET_ORDER[f])

But this would be just a workaround, really.

Copy link
Contributor Author

@miallo miallo Apr 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[look at the comment below first]
Can you think about this for a moment:
Let us define addition as (a_1⊕b_1)+(a_2⊕b_2)=(a_1+a_2)⊕(b_1+b_2) and (a_1⊕b_1)*(a_2⊕b_2)=(a_1*a_1)⊕(b_1*b_2) (a_i in A, b_i in B). Let the order of A be larger than the order of B. Therefore the mapping (a,b)->a should (according to my calculations for GF(2)⊕GF(3) (yes, I wrote down the tables 😂) and some thoughts) lead to a valid field again. And since all finite fields are isomorphic to Galois Fields then the implementation of contains_set(self, other) for them should just return self.order >= other.order and the addition works perfectly.

From my point of view this would be the "obvious" addition for fields.

Copy link
Contributor Author

@miallo miallo Apr 30, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point I was trying to make is that I think, that this diagram commutes. And F is also a homomorphism if the order of the isomorphic Galois field of A is larger, than of B:
diagram
(But the days of my maths classes are all long gone)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see... The spaces are homomorph but not isomorph. You were right all along...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤣 You went on quite a journey there, my mention of finite fields was more like an example.. In some sense you proved my minimal point that it's not trivial to implement set addition and keep it in line with mathematics for arbitrary fields.

But it doesn't seem to be necessary here anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know. But it was fun to try to remember all the maths stuff I learned 5 years ago and never used again. And despite (or maybe because) studying physics I sometimes enjoy to do "recreational maths" 😂


def __radd__(self, other):
if other == 0:
return self
else:
return self.__add__(other)


class ComplexNumbers(Field):

Expand Down Expand Up @@ -428,10 +447,7 @@ def __hash__(self):

def element(self, inp=None):
"""Return a real number from ``inp`` or from scratch."""
if inp is not None:
return float(inp)
else:
return 0.0
return float(getattr(inp, 'real', 0.0))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the way to go?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't do it this way. The issue is that this maps anything which hasn't got a .real attribute to 0.0. Core types like int or bool will play nicely with this, but anything else that could be mapped to a float wouldn't. For instance strings or custom classes that implement __float__, but not real.

So I'd prefer this:

if inp is None:
    return 0.0
else:
    return float(getattr(inp, 'real', inp))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hadn't thought about that... That is a nice solution!


@property
def examples(self):
Expand Down
Loading