Skip to content

Fix: Handle Missing Extinction Coefficient Data by Defaulting to Zero#77

Merged
HarrisonKramer merged 5 commits intoHarrisonKramer:masterfrom
manuelFragata:bugfix/extinction_coeff
Feb 22, 2025
Merged

Fix: Handle Missing Extinction Coefficient Data by Defaulting to Zero#77
HarrisonKramer merged 5 commits intoHarrisonKramer:masterfrom
manuelFragata:bugfix/extinction_coeff

Conversation

@manuelFragata
Copy link
Collaborator

Description

This PR addresses the issue where the MaterialFile.k() function raises a ValueError when no extinction coefficient data is found in the material file (e.g., with Malitson.yml - this came up when I was using the "fused_silica" material, for example).

Proposed Fix

  • Updated the k() Method in material_file.py:
    • Data Check:
      The method now checks whether self._k or self._k_wavelength is None. If one of them is missing, the function will set a default extinction coefficient of zero.
    • Warning Message:
      A print statement has been added to notify the user, because it may be important for some of their analyses:

      "WARNING: Extinction coefficient not found. It is assumed to be 0"

    • Return Values:
      • If the input wavelength is a scalar, the method returns 0.0.
      • If the input wavelength is a NumPy array, it returns an array of zeros matching the shape of the input.
    • Interpolation:
      If the extinction data is present, the method uses np.interp to interpolate the extinction coefficient for the given wavelengths, as the already implemented approach.

Testing

Here is the warning being triggered, but it allows the script to continue running:

image

Additional Modifications

I would also need some suggestions on how to show this warning message only once. Right now it will show up multiple times, as many as the method k() is called.
Any ideas on how I can improve this?

Thanks :)
Manuel

@manuelFragata
Copy link
Collaborator Author

I think it failed these tests because it did not raise the exception error (that I removed). Suggestions?

@HarrisonKramer
Copy link
Owner

Hi Manuel,

Thanks for opening this!

Yes, the failure is expected, so no problem. Those tests should either be removed or updated to catch the warning output. The latter is not trivial, though I do it elsewhere in the tests, I believe.

I used a similar method in materials.Material when no exact material match is found - it simply prints a warning using print. However, I wonder if we should make warnings more robust and actually use warnings. This would enable you to have this warning raised only once - otherwise I don't think it's possible (at least not easily, as far as I know). See docs for warnings.

Simple example:

import warnings
warnings.simplefilter("once", UserWarning)

One other nice thing about using warnings is that users can also ignore them, if they wish. I am not certain of the best approach, but I'm inclined to use warnings, especially in your case when the print statement appears multiple times (could be 100s for some operations). If we go this route, then I'd want to change the warning in the materials module too, though that doesn't need to be in this PR.

What do you think?

Regards,
Kramer

Copy link
Owner

@HarrisonKramer HarrisonKramer left a comment

Choose a reason for hiding this comment

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

Looks good. This approach works well for now.

Can you delete the failing tests and make the minor change I suggested?

Then we can merge 👍

Kramer

@manuelFragata
Copy link
Collaborator Author

Hi Kramer,

Thank you for the feedback!

I agree, we can use warnings in the future, to be more robust. I did manage to find a quick solution to printing the warning only once. I simply created a flag self.k_warning_printed that if True, then the warning will be printed only once, which is already a lot better. I wouldn't mind working on this new approach for warnings in the material module, so I can open another PR.

Thank you!

Manuel

@codecov
Copy link

codecov bot commented Feb 22, 2025

Codecov Report

Attention: Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
optiland/materials/material_file.py 90.00% 1 Missing ⚠️

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #77      +/-   ##
==========================================
- Coverage   96.57%   96.56%   -0.02%     
==========================================
  Files         105      105              
  Lines        6340     6345       +5     
==========================================
+ Hits         6123     6127       +4     
- Misses        217      218       +1     
Files with missing lines Coverage Δ
optiland/materials/material_file.py 98.70% <90.00%> (-0.63%) ⬇️

@HarrisonKramer
Copy link
Owner

Thanks a lot for the contribution, @manuelFragata! and thanks for volunteering to work on warnings in the material module.

PR looks good. Merging now!

Regards,
Kramer

@HarrisonKramer HarrisonKramer merged commit 1e88641 into HarrisonKramer:master Feb 22, 2025
6 checks passed
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.

2 participants