WIP: move nullspace to mat #47
Conversation
|
@jkruzik May I ask you for a preliminary review? I will finish things like writing manpages and finishing some of the TODOs once the concept is agreed. |
haplav
force-pushed
the
haplav/move-nullspace-to-mat
branch
from
87913bf
to
2f6ab7a
Compare
haplav
force-pushed
the
haplav/move-nullspace-to-mat
branch
from
4286796
to
112b61d
Compare
This comment has been minimized.
This comment has been minimized.
haplav
force-pushed
the
haplav/move-nullspace-to-mat
branch
from
09c7296
to
738ff88
Compare
OK, done now in fb2f271. Note also |
haplav
force-pushed
the
haplav/move-nullspace-to-mat
branch
from
fb2f271
to
357f4f8
Compare
|
(partial squash) |
|
TODO
|
| TRY( MatInvSetNullSpace(Kplus,R) ); | ||
| } else { | ||
| TRY( MatGetNullSpaceMat(K,&R) ); | ||
| if (!R) { |
There was a problem hiding this comment.
Can lead to a wrong result, if K changes after a call to MatSetNullSpaceMat(). We need to ensure that R is a valid nullspace as I was saying throughout #47 (comment). My suggestion is to store K ObjState on Set().
There was a problem hiding this comment.
I agree this is an issue (quite hypothetical at the moment, though).
However, as you can see in diff, view, this issue is not introduced by this PR. It was there before, and it's not a goal of this PR to solve it. It's more convenient for me to fix everything about nullspace use in QPTDualize in #20 and fits its scope better. And there will probably be another discussion how exactly all this will be implemented.
There was a problem hiding this comment.
mentioned now in TODOs #20 (comment)
There was a problem hiding this comment.
Though, the bug is introduced by this PR. We had QPSetNullSpaceOperator() and therefore users knew that QP object uses R - changing K after setting R to QP is a bug in user code. Now, R is hidden in K and users have no idea that QP uses R.
Almost all of the comments from my point of view in #47 (comment) were about solving this bug regardless of superficial naming and benchmarking stuff. It should be solved here as it is not related to the singular Hessian in QPTDualize.
As was pointed out many times, there is an easy solution with ObjState tracking.
There was a problem hiding this comment.
Though, the bug is introduced by this PR.
This is funny. First of all, the "bug" is not even covered by tests. So it's rather hypothetical. Feel free to add a few tests in #20 which will make this bug reproducible and I will have to fix it in order to make tests passing.
We had QPSetNullSpaceOperator() and therefore users knew that QP object uses R - changing K after setting R to QP is a bug in user code. Now, R is hidden in K and users have no idea that QP uses R.
If the user has no idea, then she/he probably doesn't set the nullspace at all and then it gets computed automatically in QPTDualize with correct results. No change here.
If additionally they change K after QPTDualize and then call QPTDualize again, they get wrong results. No change here. [Quite unlikely use case as of now, though.]
It the user explicitly sets/computes nullspace, it is intentional act on their side and they take the responsibility. No change here.
And if they change the operator afterwards, then they get wrong results. No change here.
There was never before a mechanism to really protect from these issues. Stating that it had been a user code bug and now it is our bug, just because R is now directly a K property, is ridiculous. And it's the more so fishy argumentation that it has never been properly explained in docs so it's always our fault actually.
However, there could have been problems with syncing R in MATINV and R in QP before, which are now gone at least.
As was pointed out many times, there is an easy solution with ObjState tracking.
I don't question it. But it's not the only solution. I asked you to postpone the discussion to #20 and you once again make me to add some reasons for it, which is quite frustrating, but OK.
We could as well make really sure the provided nullspace is correct by calling CheckNullSpaceMat in QPTDualize - its cost is a few purely local matrix-vector products and maybe it would pay off to do that rather than rely on ObjState or so. I don't advocate that right now and I don't wanna discuss it here. I need to think about that. And it's a matter of #20 which will follow.
Could you please let me do things in my preferred order since I'm the one who will implement it eventually? I promised I would solve it in #20. I thought that should normally suffice. So if you let me do my job, I will finish the TODOs and it will be ready for merge.
There was a problem hiding this comment.
I probably did not make myself clear, what I want to know is why do you believe it is better to fix it in #20 and not here. My reasoning is that it clearly falls into the scope of this PR while it has nearly nothing to do with non-singular Hessian in QPTDualize. The other thing is that merging something with a known fault with a view of hopefully fixing it down the line is a bad idea. To reiterate, it is possible to solve it in #20 if there is a good reason to do it there and it falls into its scope (I really struggle to see the latter point).
Another solution, which would suffice and is the simplest, is to leave this as is, but then it should be mentioned in the doc to QP object and QPTDualize that it uses R attached using MatSetNullSpaceMat() to the operator set by QPSetOperator().
At first, I wanted to end my reply here and skip replying to most of the things you raised because it is actually not related to what I need to know from you. Clearly, whether this problem stems from the changes in this PR (which has nothing to do with a simple diff but rather the composition of the objects) can be contested, but ultimately the problem is here, it was found due to this PR and is clearly related to it. However, I do feel obligated to clear some of the points and comment on some of your reasoning that I considered flawed.
This is funny. First of all, the "bug" is not even covered by tests. So it's rather hypothetical.
Saying that a problem is not covered by a test is irrelevant - it is still a problem
Feel free to add a few tests in #20 which will make this bug reproducible and I will have to fix it in order to make tests passing.
I am not going to put code directly into your PR unless you specifically ask me to do so. However, if I were to do that I could choose to add the aforementioned test into this PR.
We had QPSetNullSpaceOperator() and therefore users knew that QP object uses R - changing K after setting R to QP is a bug in user code. Now, R is hidden in K and users have no idea that QP uses R.
If the user has no idea, then she/he probably doesn't set the nullspace at all and then it gets computed automatically in QPTDualize with correct results. No change here.
I said that, with these changes, the user does not know that setting null space into K affects operations on QP object. There is a fundamental difference between
QPSetOperatorNullSpace(QP,Mat)
which operates on QP objects and
MatSetNullSpaceMat(Mat,Mat),
because in the latter there is no obvious connection to QP. That should certainly be mentioned in the doc.
If additionally they change K after QPTDualize and then call QPTDualize again, they get wrong results. No change here. [Quite unlikely use case as of now, though.]
I never mentioned this anywhere.
It the user explicitly sets/computes nullspace, it is intentional act on their side and they take the responsibility. No change here.
There was never before a mechanism to really protect from these issues. Stating that it had been a user code bug and now it is our bug, just because R is now directly a K property, is ridiculous. And it's the more so fishy argumentation that it has never been properly explained in docs so it's always our fault actually.
Again the difference is that with setting the nullspace to Mat there is no obvious connection to QP.
However, there could have been problems with syncing R in MATINV and R in QP before, which are now gone at least.
Yes, that is good although besides the point.
As was pointed out many times, there is an easy solution with ObjState tracking.
I don't question it. But it's not the only solution. I asked you to postpone the discussion to #20 and you once again make me to add some reasons for it, which is quite frustrating, but OK.
Yes, I did want some good reasons to postpone this to #20, and as a reply, I did get a very long message that does not give any reason to postpone it. You just argued why this problem is similar to the state before the change.
I promised I would solve it in #20. I thought that should normally suffice.
No. What suffices is a clear reason why it should be done in #20.
No description provided.