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 active medium in solver2x2 #136

Merged
merged 4 commits into from
May 3, 2023

Conversation

MarJMue
Copy link
Collaborator

@MarJMue MarJMue commented Apr 19, 2023

fixes #130

@MarJMue MarJMue requested a review from domna April 19, 2023 12:40
@MarJMue MarJMue marked this pull request as ready for review April 19, 2023 12:40
@@ -55,6 +55,16 @@ def calculate(self) -> Result:
)
)

for layer in n_list:
if np.any(np.logical_and(layer.real > 0, layer.imag < 0)):
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't n < 0 and k > 0 not also be allowed. I think we are fixed to work in n+ik, right? So we cannot easily switch signs and would interpret this as absorption + negative refractive index. Whether we can handle negative refractive index is the other question 😅. But maybe we should throw two distinct error messages for this case? One that it cannot handle active media and one that it cannot handle negative refractive indices?

Your decision, I would be happy with both solutions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I suppose you are right.
One other thing: Did we prefer an error or a warning?

Copy link
Member

Choose a reason for hiding this comment

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

I think we never really discussed what it should be. We both assumed that it would be an error.

However, after thinking about it I think it might be better to make it a warning. My view here is that an error should only be thrown if the problem is unrecoverable and the program cannot continue with this value. This is not the case for us. Also I find it always harsh when I program quits with an error and typically it's nicer to get weird data + a warning which tells you how to resolve the problem.

Copy link
Member

Choose a reason for hiding this comment

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

Btw, if you make it a warning. Do we want start using logging for this stuff?

I don't know if we have some other information we display somewhere, but I think we never really outputted a warning or so...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok then we agree, a warning is best.

I never used logging, but the dedicated standard library is most of the time the best way to do something like this. I will read into it.
Maybe this is also a good way to print debugging information instead of wrinting printing statements into the code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have decided to use the warning package for now, as long as we do not use any other logging features.

@MarJMue MarJMue merged commit ae233a6 into master May 3, 2023
@MarJMue MarJMue deleted the solver2x2-active-media-check branch May 3, 2023 06:36
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.

Solver2x2 does not work with active media
2 participants