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

add check for A not linop in prox #220

Merged
merged 3 commits into from
Feb 11, 2022

Conversation

tbalke
Copy link
Contributor

@tbalke tbalke commented Feb 10, 2022

See #215

Note that check for self.has_prox, namely that W is Diagonal, was redundant and so I removed it.

@tbalke
Copy link
Contributor Author

tbalke commented Feb 10, 2022

In test_functional.py we get the following functions failing:

    def test_squared_l2(self):
        L = loss.SquaredL2Loss(y=self.y, A=self.Ao)
        assert L.has_eval == True
        assert L.has_prox == False  # not diagonal

    def test_weighted_squared_l2(self):
        L = loss.WeightedSquaredL2Loss(y=self.y, A=self.Ao, W=self.W)
        assert L.has_eval == True
        assert L.has_prox == False  # not diagonal

with

        assert L.has_eval == True
>       assert L.has_prox == False  # not diagonal
E       AssertionError: assert True == False
E        +  where True = <class 'scico.loss.WeightedSquaredL2Loss'>\nhas_eval = True\nhas_prox = True\n        .has_prox

However, self.Ao is a LinearOperator so in this case we indeed have a prox. Why are we asserting that has_prox is False?

@bwohlberg
Copy link
Collaborator

Note that check for self.has_prox, namely that W is Diagonal, was redundant and so I removed it.

In what way was it redundant?

@bwohlberg bwohlberg closed this Feb 10, 2022
@bwohlberg bwohlberg reopened this Feb 10, 2022
@tbalke
Copy link
Contributor Author

tbalke commented Feb 11, 2022

Note that check for self.has_prox, namely that W is Diagonal, was redundant and so I removed it.

In what way was it redundant?

We had

if isinstance(self.A, linop.Diagonal) and isinstance(self.W, linop.Diagonal):
    self.has_prox = True

At this point isinstance(self.W, linop.Diagonal) == True always because we set it to a Diagonal in case it is None.

@bwohlberg
Copy link
Collaborator

At this point isinstance(self.W, linop.Diagonal) == True always because we set it to a Diagonal in case it is None.

But what if W is not None, but is set to something other than a linop.Diagonal?

@tbalke
Copy link
Contributor Author

tbalke commented Feb 11, 2022

At this point isinstance(self.W, linop.Diagonal) == True always because we set it to a Diagonal in case it is None.

But what if W is not None, but is set to something other than a linop.Diagonal?

Then raise TypeError(f"W must be None or a linop.Diagonal, got {type(W)}")

So it is either Diagonal or None in which case we set it to Identity which is a Diagonal

@bwohlberg
Copy link
Collaborator

However, self.Ao is a LinearOperator so in this case we indeed have a prox. Why are we asserting that has_prox is False?

Previously (some months ago), the prox was only defined if the linear operator was diagonal. That changed when you (I think) added a CG solution for non-diagonal case. I would have assumed that this test was a remnant from before, but that doesn't explain why it only started failing now.

@bwohlberg
Copy link
Collaborator

But what if W is not None, but is set to something other than a linop.Diagonal?

Then raise TypeError(f"W must be None or a linop.Diagonal, got {type(W)}")

So it is either Diagonal or None in which case we set it to Identity which is a Diagonal

OK, that makes sense. The full context is not apparent in the diff!

@tbalke
Copy link
Contributor Author

tbalke commented Feb 11, 2022

However, self.Ao is a LinearOperator so in this case we indeed have a prox. Why are we asserting that has_prox is False?

Previously (some months ago), the prox was only defined if the linear operator was diagonal. That changed when you (I think) added a CG solution for non-diagonal case. I would have assumed that this test was a remnant from before, but that doesn't explain why it only started failing now.

Correct. I think that the test fails because this is not an appropriate test. I suggest changing the test to the negation. If the A is a linop then has_prox should be true.

@bwohlberg
Copy link
Collaborator

Correct. I think that the test fails because this is not an appropriate test. I suggest changing the test to the negation. If the A is a linop then has_prox should be true.

That seems reasonable, but I'm a bit concerned that we don't understand why the tests only started failing now.

@tbalke
Copy link
Contributor Author

tbalke commented Feb 11, 2022

Correct. I think that the test fails because this is not an appropriate test. I suggest changing the test to the negation. If the A is a linop then has_prox should be true.

That seems reasonable, but I'm a bit concerned that we don't understand why the tests only started failing now.

The previous test asserted that has_prox == False (incorrectly) and the loss.py code set the has_prox attribute to False (incorrectly) when A was a linop.

Fixing the loss.py code made fixing the test code necessary.

@bwohlberg
Copy link
Collaborator

That makes sense w.r.t. tests, but it is a bit strange that we did not previously noticed that the new prox capability that was added was crippled by this oversight. Nevertheless, you may as well go ahead and make the proposed changes to the tests.

@codecov
Copy link

codecov bot commented Feb 11, 2022

Codecov Report

Merging #220 (88a67f3) into main (912b460) will decrease coverage by 0.02%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #220      +/-   ##
==========================================
- Coverage   92.44%   92.42%   -0.03%     
==========================================
  Files          49       49              
  Lines        3429     3431       +2     
==========================================
+ Hits         3170     3171       +1     
- Misses        259      260       +1     
Flag Coverage Δ
unittests 92.42% <66.66%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
scico/loss.py 88.34% <66.66%> (-0.76%) ⬇️

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 912b460...88a67f3. Read the comment docs.

Copy link
Collaborator

@bwohlberg bwohlberg left a comment

Choose a reason for hiding this comment

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

Looks good, but let's ask @Michael-T-McCann to take a look too.

@bwohlberg bwohlberg linked an issue Feb 11, 2022 that may be closed by this pull request
Copy link
Contributor

@Michael-T-McCann Michael-T-McCann left a comment

Choose a reason for hiding this comment

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

While it still strikes me as odd every time we call CG "a prox," this PR seems in keeping with the currently design.

@tbalke tbalke merged commit 008b436 into main Feb 11, 2022
@tbalke tbalke deleted the thilo/WeightedSquaredL2Loss_assumptions branch February 11, 2022 17:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unchecked assumptions on attribute type
3 participants