-
Notifications
You must be signed in to change notification settings - Fork 42
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
Adding jones adapter, docs, vortex retarder, jones-to-mueller conversion #111
Conversation
supported functions: focus, unfocus, angular spectrum, focus_fixed_sampling, unfocus_fixed_sampling
prysm/x/polarization.py
Outdated
supported_propagation_funcs = ['focus','unfocus','focus_fixed_sampling','angular_spectrum'] | ||
|
||
|
||
U = np.array([[1, 0, 0, 1], |
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.
Please create this where it is needed (at most with a def _U():; U = _U()
) -- by pre-computing it like this, GPU support is broken, since it will always reside on the device where "np" is at import time, which is always numpy-numpy.
Please also add dtype=config.precision_complex
to the array()
call
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.
Moved into jones_to_mueller
and added the dtype=config.precision_complex
prysm/x/polarization.py
Outdated
@@ -273,31 +286,108 @@ def linear_polarizer(theta=0, shape=None): | |||
|
|||
return linear_diattenuator(0, theta=theta, shape=shape) | |||
|
|||
def vector_vortex_retarder(charge, shape, retardance=np.pi, theta=0): |
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.
taking shape
as an input is not prysm-like; please take appropriate coordinate arrays and do not make a grid internally
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’ll have it take in an angular coordinate theta
and update the tutorial to include it
prysm/x/polarization.py
Outdated
|
||
@functools.wraps(prop_func) | ||
def wrapper(*args,**kwargs): | ||
|
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.
delete two extra white spaces
prysm/x/polarization.py
Outdated
if name in funcs_to_change: | ||
setattr(propagation, name, jones_adapter(func)) | ||
|
||
def apply_polarization_to_field(field): |
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.
This is not used in the module and does not have an intuitive mapping between its name and what it does; please remove
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.
From the Polarized_Propagation.ipynb
demo:
This was added in response to a comment on #91 regarding the demo containing A = A[…, np.newaxis, np.newaxis]
to enable element-wise multiplication. Would you prefer apply_polarization_component_to_field
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.
It is a design question, I think
I understand that vvr * A
is a very nice syntax since it reads like the calculus
What I meant was a function ~=
def apply_polarization_optic(field, pol):
# internal logic understands 2D -> 4D broadcasts
But this is fairly verbose if it is going to be used all the time
Perhaps instead the routines that create polarization components could return wavefronts, and the math overrides on wavefronts could be extended to include the newaxis's when there is a dimensionality mismatch?
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 apologies I misunderstood. I can add apply_polarization_optic
now and we can work on adding support for Wavefronts in a future PR?
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.
Added in 4dbd050
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 change in 4dbd050
is not quite right; in the event that the field is already polarization compatible, it will make it (N,M,2,2,1,1)
; please add an if field.ndim == 2
check (or != 4, up to you)
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.
change made in b885996
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.
This is differently buggy; you need to add two dummy dimensions if the field is 2D. This does that, but then only applies pol_optic if the field was 2D.
Change made in 558aad3 |
merged :) |
No description provided.