SyneRBI / SIRF-Exercises

SIRF Training and demonstration material
http://www.ccpsynerbi.ac.uk
Apache License 2.0
18 stars 21 forks source link

Corrections for geometry notebook #137

Closed johannesmayer closed 3 years ago

johannesmayer commented 3 years ago

Two corrections are applied. Fixes #136.

johannesmayer commented 3 years ago

I already know that there are all these weird changes aside from the one I actually made, but for the love of God I could not get them to disappear using this nbstripout shabang.

KrisThielemans commented 3 years ago

I already know that there are all these weird changes aside from the one I actually made, but for the love of God I could not get them to disappear using this nbstripout shabang.

I can't get rid of them either for some reason... Can you nbstripout --uninstall, then in Jupyter notebook say "clear all output" or even restart. then save/checkpoint, a git diff should then show some stuff. Push again. nbstripout --install. Weird.

Otherwise, this PR looks good to me

DANAJK commented 3 years ago

I have not had a chance to re-run the notebook, but I believe my original one is correct. The 'transformation' is an element by element product which I think is equivalent to your proposal of a matrix multiplication by diag([-1 -1 1 1]). I need to think carefully about what is clearest to the reader - I can see the appeal of using matrix multiplication. Element by element multiplication is commutative, but I can see it looks confusing.

On the spacing, the norm should return the sqrt-sum-of-squares so it should not need a further sqrt. I would need to go back and check everything, but on the face of it, my original looks correct to me.

johannesmayer commented 3 years ago

Oh I see now, you are right that your RAH2LPH matrix multiplied element-wise is the same as doing the matrix multiplication.

Since the RAH2LPH is a change in coordinate systems I would very much suggest the use of the matrix multiplication since that's what the whole notebook is about. And I think it connects nicely with the notation of getting (in Matlab notation):

( v_l, v_p, v_h) = RAH2LPH * A_RAH * (i, j, k, 1).

whereas when you do it element-wise it has to be

( v_l, v_p, v_h) = RAH2LPH .* A_RAH * (i, j, k, 1)

which I found very strange.

As for the normalisation, you are right, I got confused by the output

image

s.t. I thought this can't be a unit vector but it was just bad display, I will revert that.

KrisThielemans commented 3 years ago

I also vote for matrix multiplication I'm afraid. @johannesmayer I guess you checked that both gave the same result?

johannesmayer commented 3 years ago

Yes, I did check that it gave the same result for A_LPH for using @DANAJK matrix with the element-wise multiplication, as well as the matrix multiplication from this PR. Hence, all the numbers afterwards are identical, too, and I went through and compared and I understood all that followed.

KrisThielemans commented 3 years ago

ok. thanks, can you do the "clear output" stuff please?

johannesmayer commented 3 years ago

I tried the command sequence you proposed, and I was very careful of not having any output in my cells and clearing it before even the first commit in here, but it just would always change these cell ids and there is nothing I could do that prevented them form appearing.

I can delete this branch, do a new one from master and try again but I don't think it will work.

johannesmayer commented 3 years ago

Yeah, same result, I did not even RUN any code, just opened the notebook, changed a line and saved it, and it will add all these cell-id's...

ashgillman commented 3 years ago

When you are adding changes, I like to use git add --patch, which goes through chunks of code and asks one-by-one if you'd like to commit. Then you can be very careful over what your are commiting

johannesmayer commented 3 years ago

Cheers dude, now I got git down to two lousy cell id-s and that's as much as they are willing to give me apparently.

DANAJK commented 3 years ago

Looks OK.