Changing construction of the first guess and coefficient matrices in NLTE solver #2201
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Co-authored-by: Christian Vogl <cvogl@mpa-garching.mpg.de>
…TE excitation too
Codecov Report
@@ Coverage Diff @@
## master #2201 +/- ##
==========================================
+ Coverage 71.76% 71.96% +0.19%
==========================================
Files 133 137 +4
Lines 12344 12514 +170
==========================================
+ Hits 8859 9006 +147
- Misses 3485 3508 +23
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
sonachitchyan
commented
Feb 23, 2023
chvogl
reviewed
Apr 19, 2023
| Matrix with excitation-deexcitation rates(should be added to NLTE rate matrix for excitation treatment). | ||
| NOTE: only works with ONE ion number treated in NLTE excitation AT ONCE. | ||
| """ | ||
| number_of_shells = beta_sobolev.shape[1] |
There was a problem hiding this comment.
I still think it would be better to have this method operate on a single shell. I don't think we want to set up multiple rate matrices at once. The main advantage would be that is simplifies the code. However, since it likely will not affect the performance, we can also leave it as it is.
There was a problem hiding this comment.
I can leave it for now, and when we have the full implementation, see if switching it slows down the code
Co-authored-by: Christian Vogl <cvogl@mpa-garching.mpg.de>
Co-authored-by: Christian Vogl <cvogl@mpa-garching.mpg.de>
Co-authored-by: Christian Vogl <cvogl@mpa-garching.mpg.de>
…rdis-sn/tardis into excitation_first_guess_implementation
Co-authored-by: Christian Vogl <cvogl@mpa-garching.mpg.de>
Co-authored-by: Christian Vogl <cvogl@mpa-garching.mpg.de>
…rdis-sn/tardis into excitation_first_guess_implementation
Rodot-
previously approved these changes
Apr 26, 2023
chvogl
reviewed
May 3, 2023
| Matrix with excitation-deexcitation rates(should be added to NLTE rate matrix for excitation treatment). | ||
| NOTE: only works with ONE ion number treated in NLTE excitation AT ONCE. | ||
| """ | ||
| number_of_shells = beta_sobolev.shape[1] |
Co-authored-by: Christian Vogl <cvogl@mpa-garching.mpg.de>
…rdis into excitation_first_guess_implementation
…rdis-sn/tardis into excitation_first_guess_implementation
chvogl
approved these changes
May 3, 2023
Rodot-
approved these changes
May 3, 2023
light2802
pushed a commit
to light2802/tardis
that referenced
this pull request
May 27, 2023
…NLTE solver (tardis-sn#2201) * implementing nlte_excitation * ran black * fixed a typo * got rid of unnecessary lines * rewrote tests * made a change on assigning the config values to plasma properties * fixed the tests * Update tardis/io/tests/test_config_reader.py Co-authored-by: Christian Vogl <cvogl@mpa-garching.mpg.de> * restructured the first guess for NLTE solver to be compatible with NLTE excitation too * ran black * modified the existing rates to change when treating species with NLTE excitation * nlte not implemented, remove from rate_matrix_index * ran black * fixed an issue with one of the tests * attempt of adding bound bound interaction coefficients * fixed an issue with bound bound rates matrix * added docstrings * ran black * changed one of the methods to static * added test for bound bound rates part of the matrix * ran black * added docstrings to nlteexcitationdata * got rid of unnecessary import * added a new parameter to the docsting of the method * got rid of the number conservation row for the bound-bound matrix as it will be added on in the overall matrix construction * ran black * remodified the tests * fixed a loop issue * added a todo comment * renamed a function * updated a docstring * Update tardis/plasma/properties/nlte_rate_equation_solver.py Co-authored-by: Christian Vogl <cvogl@mpa-garching.mpg.de> * Update tardis/plasma/properties/nlte_rate_equation_solver.py Co-authored-by: Christian Vogl <cvogl@mpa-garching.mpg.de> * Update tardis/plasma/properties/nlte_rate_equation_solver.py Co-authored-by: Christian Vogl <cvogl@mpa-garching.mpg.de> * got rid of t_electrons * resolved an issue with tests * added additional information into a docstring * added a docstring * got rid of unnecessary grouping of rates * Update tardis/plasma/properties/nlte_rate_equation_solver.py Co-authored-by: Wolfgang Kerzendorf <wkerzendorf@gmail.com> * fixing a minor issue in tests * separated nlte excitation coefficients * running black * separating beta sobolev related stuff from the rest of the bound bound matrix * Update tardis/plasma/properties/nlte_rate_equation_solver.py Co-authored-by: Christian Vogl <cvogl@mpa-garching.mpg.de> * factored prepare_bound_bound_rate_matrix * removed an unnecessary function for excitation rates * removed unnecessary method for concatinating rates for NLTE excitation * ran black on nlte_rate_equation_solver * got rid of an unnecessary method * fixed a typo * small fixed * modified the j_blues and beta_sobolevs used in the tests * running black once again * adding rate_matrix_index to first guess docstring * Update tardis/plasma/properties/nlte_rate_equation_solver.py Co-authored-by: Christian Vogl <cvogl@mpa-garching.mpg.de> * Update tardis/plasma/properties/nlte_rate_equation_solver.py Co-authored-by: Christian Vogl <cvogl@mpa-garching.mpg.de> * Update tardis/plasma/tests/test_nlte_excitation.py Co-authored-by: Christian Vogl <cvogl@mpa-garching.mpg.de> * fixed a typo * switched from using the atomic_data_levels to number_of_levels * Update tardis/plasma/properties/nlte_rate_equation_solver.py Co-authored-by: Christian Vogl <cvogl@mpa-garching.mpg.de> * Update tardis/plasma/properties/nlte_rate_equation_solver.py Co-authored-by: Christian Vogl <cvogl@mpa-garching.mpg.de> * fixed the issue with tests * ran black once again * left a comment for future generations of NLTE youth * ran black once again * Update tardis/plasma/properties/nlte_rate_equation_solver.py Co-authored-by: Christian Vogl <cvogl@mpa-garching.mpg.de> * got rid of an unnecessary argument --------- Co-authored-by: Christian Vogl <cvogl@mpa-garching.mpg.de> Co-authored-by: Wolfgang Kerzendorf <wkerzendorf@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
📝 Description
Type: 🪲
bugfix| 🚀feature| ☣️breaking change| 🚦testing| 📝documentation| 🎢infrastructureThis PR makes sure that thewayt coefficient matrices and the first guess of NLTE solver are constructed is able to work both with and without NLTE excitation species.
Also, link issues affected by this pull request by using the keywords:
close,closes,closed,fix,fixes,fixed,resolve,resolvesorresolved.📌 Resources
Examples, notebooks, and links to useful references.
🚦 Testing
How did you test these changes?
☑️ Checklist
build_docslabel