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

Implement astropy unit conversion to rescale_path_length() #445

Merged
merged 3 commits into from Apr 8, 2022
Merged

Implement astropy unit conversion to rescale_path_length() #445

merged 3 commits into from Apr 8, 2022

Conversation

TranHuuNhatHuy
Copy link
Collaborator

Description

This pull request is to address #444. Now we can use astropy units for the input arguments of rescale_path_length().

Beside, currently on the RADIS documentation, rescale_path_length() has 2 references:

  1. radis.Spectrum.rescale_path_length
  2. radis.Spectrum.rescale.rescale_path_length

For [1]:

  • @erwanp 's syntax is added in the Examples section.
  • I also notice that the gallery example for [1] does not show up on documentation. Currently the referring syntax is .. minigallery:: radis.Spectrum.rescale_path_length.
  • Link for the source is currently not working. You can see it here.

For [2], it's just kinda empty here.

@erwanp
Copy link
Member

erwanp commented Apr 7, 2022

Nice ! I think it should be radis.spectrum.Spectrum.rescale_path_length

there is a script in doc/gendocs.sh that allows you to generate docs locally; helps the debugging. You may need to install the doc specific requirements

@erwanp
Copy link
Member

erwanp commented Apr 7, 2022

What we'll need before merging is to add tests.

There is a test_rescale.py file. You can add a new function there, testing the units.

You can use a precomputed spectrum such as

s = load_spec(getTestFile("CO_Tgas1500K_mole_fraction0.01.spec"), binary=True)

and you could, for instance, compare the absorbance (in absorption spectroscopy, the absorbance is A = k * L where k is the absorption coefficient, L the length ; so absorbance should have changed when you rescaled the spectra ) of a a spectrum rescaled with astropy units (e.g 1 * u.m ) and with default units (100 # cm )

@codecov-commenter
Copy link

Codecov Report

Merging #445 (77f71b2) into develop (778bdbf) will increase coverage by 0.02%.
The diff coverage is 91.66%.

@@             Coverage Diff             @@
##           develop     #445      +/-   ##
===========================================
+ Coverage    77.06%   77.09%   +0.02%     
===========================================
  Files          161      161              
  Lines        18641    18664      +23     
===========================================
+ Hits         14366    14389      +23     
  Misses        4275     4275              

radis/test/spectrum/test_rescale.py Outdated Show resolved Hide resolved
radis/test/spectrum/test_rescale.py Show resolved Hide resolved
@TranHuuNhatHuy
Copy link
Collaborator Author

@erwanp thank you for the reviews. Last commit features two changes according to your suggestions:

  1. Fetch absorbance directly by using s.get("absorbance", wunit='cm-1'). I believe this is better than fetching k and L then calculate the absorbance A = k * L, in terms of computational performance and the sake of clean code.
  2. Add third assertion of comparing the two path_length of 100000 cm and 1 u.km to make sure the astropy unit works fine. Although I doubt it is necessary if the two absorbances are already equal, I think with this we can check if something is wrong with the astropy units in the future (extremely unlikely thou).

@erwanp
Copy link
Member

erwanp commented Apr 8, 2022

Excellent, merging!

@erwanp erwanp added this to the 0.12.1 milestone Apr 8, 2022
@erwanp erwanp added the interface not related to the physics of the code label Apr 8, 2022
@erwanp erwanp merged commit f6777bb into radis:develop Apr 8, 2022
@erwanp erwanp mentioned this pull request Apr 13, 2022
@TranHuuNhatHuy TranHuuNhatHuy deleted the docs-improvement branch May 7, 2022 15:37
@TranHuuNhatHuy TranHuuNhatHuy restored the docs-improvement branch May 7, 2022 15:37
@anandxkumar
Copy link
Collaborator

Fixes #444

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interface not related to the physics of the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants