-
Notifications
You must be signed in to change notification settings - Fork 0
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
Added Q functions and demos #19
Conversation
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done 👏 👏 👏 just have a few minor comments before merging, you should be able to see them as comments attached to each of the file changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this notebook doesn’t use any functions from Katsu yet, I’m thinking we should wait before including it in the website. It’s great to know that there are other packages that enable motion control though! Maybe we can either add Pylab as an optional dependency in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I'll see if I can keep it as a draft in the branch but take it out of the pull request. I could also add more about how to integrate the motion commands in a DRRP data-taking script which might be more relevant to katsu
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea - if it’s too difficult we can also leave it in and then just take it out of the index.rst
that loads it into the website.
We can also save the integrated motion control / data reduction for another PR in the future :^)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love this, just a couple of comments that I’lll include in the replies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the beginning, where you say “(for more information, see FullMuellerExample.ipynb)”, can you replace this with the following:
(for more information, see [The Full Mueller Polarimetry Demo](FullMuellerExample.ipynb))
If I wrote this correctly, it should hyperlink to the Jupyter notebook
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, there is a way to get the same result using a slightly more robust method that uses measurements of the Q Stokes parameter.
Has the relative robustness been verified in the lab? If so, we should consider a demo where you demonstrate it because that’s awesome 👀 . If not, maybe we should call this an “emerging technique” instead and cite your SPIE proceedings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As opposed to measuring a single linear polarization, this measurement takes information from a greater sampling of polarization states and is therefore more thorough.
Same comment as before, has “more thorough” been verified? If not, maybe just a comment on how we (you) are investigating its potential for greater robustness.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the last linear polarizer in the polarization state analyzer (PSA) can be replaced with a Wollaston prism to allow for Q sampling.
“Q sampling” phrasing could be strengthened, maybe “direct measurement of Q” or something similar?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of a list of zeros that you have here:
# Now feed this data into a data reduction function to see how well the matrix correlates
M = q_calibrated_full_mueller_polarimetry(theta, 0, 0, 0, 0, 0, 2, 2, M_rand)
It might be more illustrative to show what each variable is in the demo, ex:
# Initialize polarizer angles
psg_polarizer_angle = 0
psg_qwp_angle = 0
psa_qwp_angle = 0
psa_polarizer_angle = 0
psg_retardance = 0
psa_retardance = 0
I_vertical = 2
I_horizontal = 2
# Now feed this data into a data reduction function to see how well the matrix correlates
M = q_calibrated_full_mueller_polarimetry(theta,
psg_polarizer_angle,
psg_qwp_angle,
psa_qwp_angle,
psg_retardance,
psg_retardance,
I_vertical,
I_horizontal,
Mrand)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last thing is to maybe add some kind of tie-up comment at the end. Like how we are using Katsu as a platform for developing new polarimetric data reduction techniques like this one you came up with!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m a little confused by the equation. Specifically the [0, 1, 0, 0] on the left. Is this a dot product or a matrix multiplication?
Also, I think you can remove the
*
in between the matrices. The multiplication is implicit for matrix and matrix-vector multiplications.
Sorry, it should be matrix/vector multiplication. The [0, 1, 0, 0] simply isolates the Q parameter, but maybe it's simpler to leave the whole Stokes vector
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe writing it as two separate equations? Replace the stokes vector [0, 1, 0, 0] with a linear analyzer oriented along the horizontal in one equation, and along the vertical in another. Then you can write something like:
And move on to say that you build the data reduction matrix with Q
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wcmelby THE DEMO LOOKS SO GOOD
It’s not clear from the demo what the following variables are, could you add some elaboration? Presumably these are power measurements, but I’m not sure why they are 2x1 arrays.
I_vertical = np.array([1, 2]) # some numpy array, can be anything for now
I_horizontal = np.array([2, 3]) # some numpy array, can be anything for now
Also, in the following
By using the optional input for M_rand, this function simulates what the data would look like for that matrix and uses that data for the matrix reconstruction.
I think it might be a bit clearer to say “setting the keyword argument M_in
to M_rand
, this function simulates what…” or something along those lines.
After that, I think we are good to go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ooh I think these files come from a git add .
, please remove this one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ooh I think these files come from a git add ., please remove this one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ooh I think these files come from a git add ., please remove this one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ooh I think these files come from a git add ., please remove this one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I delete them or just take it out of the pull request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely take em out of the pull request. I think if you delete them they just end up re-generating when you run jupyter notebooks haha.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh man this looks pretty good, especially good job on preserving the numpy docstring style 👍
Can you move the RMS_Calculuator
and propagated_error
functions to katsu_math.py
and then import them here? Also, you can remove the polar decomposition functions and just import them from mueller.py
, we already have them there!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The polar decomposition functions I imported are the same as yours except for one line where I normalize the matrices. I remember having an issue I think when calculating the retardance if these weren't normalized. I moved these two functions to mueller.py for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see. Maybe the path forward is to add a normalization option. Something like
def decompose_depolarizer(*args, **kwargs, normalize=False)
if normalize:
return M / M[…, 0, 0]
else:
return M
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, I'll add that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment here re: tests in a future PR, but otherwise this looks great!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff! Just had one last comment on the demo, after that I think we can go ahead and approve :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wcmelby THE DEMO LOOKS SO GOOD
It’s not clear from the demo what the following variables are, could you add some elaboration? Presumably these are power measurements, but I’m not sure why they are 2x1 arrays.
I_vertical = np.array([1, 2]) # some numpy array, can be anything for now
I_horizontal = np.array([2, 3]) # some numpy array, can be anything for now
Also, in the following
By using the optional input for M_rand, this function simulates what the data would look like for that matrix and uses that data for the matrix reconstruction.
I think it might be a bit clearer to say “setting the keyword argument M_in
to M_rand
, this function simulates what…” or something along those lines.
After that, I think we are good to go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! We should consider adding tests in a future PR, but this is good for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment here re: tests in a future PR, but otherwise this looks great!
Minor typo change
It took me a minute to see the change @becca9808 made but good catch 👏 I didn’t notice that one |
Thanks for the feedback, the edits are in! @Jashcraf are you able to selectively accept changes from the pull request or is it easier if I take out the Thorlabs demo now? Also I'm not sure how to revert changes to the pycache files. |
Ah no worries I think I can do it on my end |
LGTM! |
No description provided.