Pull request to solve issue #82.

[adityabharadwaj198]

Description

This pull request is to address ... the issue here #82.
I've made the changes suggested in the comments related to the issue and pushed them to my forked repo. If anyone wants to run a test to see if the changes fix the issue, they can run this:

from radis import calc_spectrum, plot_diff, Radiance_noslit
s = calc_spectrum(1900, 2300,         # cm-1
                   molecule='CO',
                   isotope='1,2,3',
                   pressure=1.01325,   # bar
                   Tgas=700,           # K
                   mole_fraction=0.1,
                   path_length=1,      # cm
                   )
s=Radiance_noslit(s)   # keep only radiance 

import numpy as np
s._q['radiance_noslit'][0] = np.nan

# Something like this would give an error otherwise:
plot_diff(s, s*1.2, normalize=True)

Fixes #82

[erwanp]

Hello @adityabharadwaj198 , thanks for submitting!

Tests fail for the moment. Don't worry about the Code Quality Checks, we'll fix that at the end. But you should have a look at the "Test and Coverage" one: seems there is a syntax error !

Fix and re-push!

[erwanp]

in the test can you also try some other normalization options? Like, "max", "area", etc.

[adityabharadwaj198]

Sure

[minouHub]

Hi,
You even added a test! Very good :)
I just suggest you remove the lines you commented.
Minou

[adityabharadwaj198]

Hi,
You even added a test! Very good :)
I just suggest you remove the lines you commented.
Minou

Hey there, I did what you suggested and pushed changes. I also used black to format the code, hope the code quality checks pass this time.

[adityabharadwaj198]

in the test can you also try some other normalization options? Like, "max", "area", etc.

I'm a bit confused about that, I've tried setting normalization_how manually, and it works, but how can I write a method to test that? Since normalization_how is an argument of the get_residual method, and that method is called from plot_diff method, and there's no normalization_how argument in that.

[erwanp]

You're right! So far I'd include a get_residual(s) test line in the test file!

@minouHub do we want to have more normalization options in plot_diff ? ("normalize=" could be given the "area" keywords, or a tuple to normalize on a certain range, like in plot ). In any case that would be a separate Issue!

[minouHub]

Well, I've never used that in post-processing, so I'm not sure it is necessary.

[codecov-io]

Codecov Report

Merging #83 into develop will increase coverage by 0.34%.
The diff coverage is 60.00%.

@@             Coverage Diff             @@
##           develop      #83      +/-   ##
===========================================
+ Coverage    71.16%   71.51%   +0.34%     
===========================================
  Files          113      111       -2     
  Lines        13517    13545      +28     
===========================================
+ Hits          9620     9687      +67     
+ Misses        3897     3858      -39     

[adityabharadwaj198]

Hello @adityabharadwaj198 , thanks for submitting!

Tests fail for the moment. Don't worry about the Code Quality Checks, we'll fix that at the end. But you should have a look at the "Test and Coverage" one: seems there is a syntax error !

Fix and re-push!

Hey there, I fixed the 'Test and Coverage' test, it was indeed a syntax error.

[erwanp]

Well done with the improvements @adityabharadwaj198

• Looking at the coverage report I realized there are some options not covered by your current changes: around L353 :

if isinstance(normalize, tuple):

we also have some calls to .max() and .mean() that could be made nan-proof

• Also we do not handle the normalize_how == "area" case right now. Can you add it in your test first (it should crash) and then decide on a fix.
  In Nan handeling in get_residual #82 is suggested to use nantrapz (the description looks wrong btw, you may want to fix that!)

Last point is a bigger suggestion:

• I realized that the same problem (nan in a Spectrum) will arise when we want to plot a single, normalized spectrum. For instance with:

s.plot(normalize=True)

We may want to use that PR to improve that aswell. In that case I would suggest to:

• add in your test a line to plot a single spectrum that has nans (as above)
• update the spectrum.py accordingly. As you'll see there are two cases: one where we normalize with the max: easy fix, let's just use nanmax.
• in the other option the Spectrum is normalized by using the mean value on a given range (it's very useful in data analysis). By looking at the code it seems that the function uses norm which itself handles nan properly. So we should be fine. But you should add a test

s.plot(normalize=(2200, 2250))  # for instance 

[adityabharadwaj198]

if isinstance(normalize, tuple):

Hey @erwanp , so I was trying to change the max and mean method calls here also, and from what I understand, to check if it's working I should be calling get_residual like this

get_residual(s, s*1.2, var="radiance_noslit", ignore_nan=True, normalize=(2200, 2250), normalize_how="max")

So a tuple is passed as an argument and if isinstance(normalize, tuple): conditional branch can be evaluated. However when I'm doing this, I'm getting an error: ValueError: zero-size array to reduction operation fmax which has no identity.. Any suggestions?

[minouHub]

I agree with @erwanp that normalizing with a tupple is very useful. I did it last week, but I did not realize there was an option for that!!!

As @adityabharadwaj198, I have some problems using this option. Here is a simple code that does not work:

s = calc_spectrum(1900, 2300,         # cm-1
                  molecule='CO',
                  isotope='1,2,3',
                  pressure=1.01325,   # bar
                  Tgas=700,           # K
                  mole_fraction=0.1,
                  path_length=1,      # cm
                  )
s2 = calc_spectrum(1900, 2300,         # cm-1
                  molecule='CO',
                  isotope='1,2,3',
                  pressure=1.01325,   # bar
                  Tgas=1000,           # K
                  mole_fraction=0.1,
                  path_length=1,      # cm
                  )
test = get_residual(s, s2, 'radiance_noslit', normalize=(2000, 2100))

The problem comes from the units: 'cm-1' are converted to 'nm' at some points. Therefore, the line 356 in compare.py b = (w1 > wmin) & (w1 < wmax) gives only False. This is why the array is 'zero-size'.
This option should be more secure and the help re-written. I can spend more time on this, but after the 7th :)

[adityabharadwaj198]

Okay, I'll look into other suggestions @erwanp made.

[minouHub]

I tried to add my test to your branch using this: https://tighten.co/blog/adding-commits-to-a-pull-request
Can you "allow edits for maintainer" please? @erwanp, am I maintainer? ^^

[adityabharadwaj198]

allow edits for maintainer

@minouHub , I checked and it is enabled

[adityabharadwaj198]

• Also we do not handle the normalize_how == "area" case right now. Can you add it in your test first (it should crash) and then decide on a fix.
  In Nan handeling in get_residual #82 is suggested to use [nantrapz]

Fixed this. I had to use s1.get() first and then use nantrapz, and I skipped using get_integral because of the way it is written, I hope that's okay

s.plot(normalize=True)

• add in your test a line to plot a single spectrum that has nans (as above)
  Added this.
• update the spectrum.py accordingly. As you'll see there are two cases: one where we normalize with the max: easy fix, let's just use nanmax.

Yes, just replacingy.max()with y.nanmax() worked

• in the other option the Spectrum is normalized by using the mean value on a given range (it's very useful in data analysis). By looking at the code it seems that the function uses norm which itself handles nan properly. So we should be fine. But you should add a test

s.plot(normalize=(2200, 2250))  # for instance 

As you said, this worked without any problems. And I added the above as a test.

[erwanp]

I agree with @erwanp that normalizing with a tupple is very useful. I did it last week, but I did not realize there was an option for that!!!

As @adityabharadwaj198, I have some problems using this option. Here is a simple code that does not work:

s = calc_spectrum(1900, 2300,         # cm-1
                  molecule='CO',
                  isotope='1,2,3',
                  pressure=1.01325,   # bar
                  Tgas=700,           # K
                  mole_fraction=0.1,
                  path_length=1,      # cm
                  )
s2 = calc_spectrum(1900, 2300,         # cm-1
                  molecule='CO',
                  isotope='1,2,3',
                  pressure=1.01325,   # bar
                  Tgas=1000,           # K
                  mole_fraction=0.1,
                  path_length=1,      # cm
                  )
test = get_residual(s, s2, 'radiance_noslit', normalize=(2000, 2100))

The problem comes from the units: 'cm-1' are converted to 'nm' at some points. Therefore, the line 356 in compare.py b = (w1 > wmin) & (w1 < wmax) gives only False. This is why the array is 'zero-size'.
This option should be more secure and the help re-written. I can spend more time on this, but after the 7th :)

Thanks for reporting, I opened a different Issue : #85 . We'll remember to activate @adityabharadwaj198 once that Issue is fixed.

Also, you are now officially a Maintainer :)

[erwanp]

• Also we do not handle the normalize_how == "area" case right now. Can you add it in your test first (it should crash) and then decide on a fix.
  In Nan handeling in get_residual #82 is suggested to use [nantrapz]

Fixed this. I had to use s1.get() first and then use nantrapz, and I skipped using get_integral because of the way it is written, I hope that's okay

s.plot(normalize=True)

• add in your test a line to plot a single spectrum that has nans (as above)
  Added this.
• update the spectrum.py accordingly. As you'll see there are two cases: one where we normalize with the max: easy fix, let's just use nanmax.

Yes, just replacingy.max()with y.nanmax() worked

• in the other option the Spectrum is normalized by using the mean value on a given range (it's very useful in data analysis). By looking at the code it seems that the function uses norm which itself handles nan properly. So we should be fine. But you should add a test

s.plot(normalize=(2200, 2250))  # for instance 

As you said, this worked without any problems. And I added the above as a test.

Well done ! I approved all changes. I'll let @minouHub approve too and then we can merge!

[minouHub]

Good to me :)

[adityabharadwaj198]

Thanks @erwanp and @minouHub !