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

Update fitting functions #336

Merged
merged 14 commits into from Aug 9, 2021
Merged

Conversation

erwanp
Copy link
Member

@erwanp erwanp commented Aug 8, 2021

Description

  • add Spectrum.sort() method
  • add 1-T fitting example
  • just-in-time tabulation of partition functions for multi-Tvib with Treanor distribution
  • update 3-T fitting example, updating fitting method
  • add download-HITEMP example

@erwanp erwanp added the interface not related to the physics of the code label Aug 8, 2021
@erwanp erwanp added this to the 0.9.30 milestone Aug 8, 2021
@erwanp erwanp requested a review from minouHub August 8, 2021 18:01
@codecov-commenter
Copy link

codecov-commenter commented Aug 8, 2021

Codecov Report

Merging #336 (4dc8f64) into develop (781fb52) will increase coverage by 0.07%.
The diff coverage is 80.17%.

@@             Coverage Diff             @@
##           develop     #336      +/-   ##
===========================================
+ Coverage    75.17%   75.24%   +0.07%     
===========================================
  Files          144      146       +2     
  Lines        15957    16050      +93     
===========================================
+ Hits         11995    12077      +82     
- Misses        3962     3973      +11     

@lgtm-com
Copy link

lgtm-com bot commented Aug 8, 2021

This pull request fixes 1 alert when merging ef47b76 into 781fb52 - view on LGTM.com

fixed alerts:

  • 1 for Redundant assignment

Copy link
Collaborator

@minouHub minouHub left a comment

Choose a reason for hiding this comment

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

  • There are multiple warnings about the iso column. I know you want to get rid of it in the near future, but just to remind you.
e:\python\radis\radis\misc\warning.py:341: PerformanceWarning: There shouldn't be a Column 'iso' with a unique value
  warnings.warn(WarningType(message))
  • In your example at Add fit spectrum method #334 (comment) : replace maxiter = 300 by solver_options={"maxiter": 300}. Although, I don't find this example anywhere in the code.
  • What is the difference between the fit_spectrum in factory.py and fitting.py. Seems odd to me to have the same function define twice.
  • Finally, this is what I get when I run the example at Add fit spectrum method #334 (comment). Not a great fit
Init ['T12', 'T3', 'Trot'] = [1150. 2650. 1150.]['', '', '']
Final ['T12', 'T3', 'Trot'] = [1150. 2650. 1150.]['', '', '']
Best ['T12', 'T3', 'Trot'] = [1150. 2670. 1150.]['', '', ''] reached at iteration 4/4

Examples
--------

.. minigallery:: radis.lbl.factory.fit_spectrum
Copy link
Collaborator

Choose a reason for hiding this comment

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

The example doesn't show anything on my side
image

Copy link
Member Author

Choose a reason for hiding this comment

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

yes ! It will only be generated on Readthedocs (once merged)

@lgtm-com
Copy link

lgtm-com bot commented Aug 9, 2021

This pull request fixes 1 alert when merging f57bb84 into 781fb52 - view on LGTM.com

fixed alerts:

  • 1 for Redundant assignment

  Add fixed parameters directly in the model  (slowly converging to a framework closer to Astropy.modelling/lmfit)
- fix residual figure not updating correctly during fits
- plot is False by default in fits
- updated example to run 120 iterations (longer; but nicer fit)
@erwanp
Copy link
Member Author

erwanp commented Aug 9, 2021

Thanks for the review!

  • fit_spectrum and fitting.py --> the 1st is just a wrapper to the 2nd. The idea is to make it easier to use fitting.py with other codes; as long as they have a Factory with similar syntax. Think of a SpecairFactory for pyspecair : )
  • I fixed all the rest. Even allowed more iterations in multi-temperature fit eventually converge to a nice fit. My only concern was that it was very long ; but your feedback convinced me it's better to show a good fit than save 1min of computation on Readthedocs.

Note : fitting is very long because parsum_mode="tabulation" cannot be activated as it is stuck with vaex. I hope to fix that (will make it 10x faster!). It's also long because the fitting algorithms (low level scipy.optimize) are not necessarily optimized. I want to switch to astropy.modelling or lmfit to have higher-level; more optimized control of the fits

@erwanp erwanp merged commit b06347a into radis:develop Aug 9, 2021
@erwanp erwanp added the post-process Related to the post-processing (ex: methods of Spectrum objects) label Aug 9, 2021
@erwanp erwanp deleted the add/update-fitting-functions branch August 9, 2021 10:08
@lgtm-com
Copy link

lgtm-com bot commented Aug 9, 2021

This pull request fixes 1 alert when merging ce26cc1 into 781fb52 - view on LGTM.com

fixed alerts:

  • 1 for Redundant assignment

@erwanp erwanp mentioned this pull request Aug 25, 2021
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 post-process Related to the post-processing (ex: methods of Spectrum objects)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants