-
Notifications
You must be signed in to change notification settings - Fork 24
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
Add ThermalCXLine model #386
Add ThermalCXLine model #386
Conversation
…dard ADF15 files.
I've added tests, demos and documentation, so I think this PR is ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @vsnever,
I'm sorry I was delaying this. I don't want to be delaying the review any further, so I'm posting this review without thesting the actual loading of the ADAS file, but I guess if there were any problems you would discover them. I have some questions and suggestions to consider which could improve the code. In any case, thanks for adding the thermal cx rate.
…y will be added in a separate PR.
Thanks a lot for the review, @Mateasek. I agree with all your suggestions. They are implemented in the latest commits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, couple of suggestions.
I appreciate the additional test coverage of all the line emission models. In the other PR we did request that ImpactExcitation
and Recombination
docstrings were moved to a separate PR: in principle the same applies here. But I've no qualms with accepting this PR as is: any increase in test coverage is to be welcomed!
world = World() | ||
|
||
atomic_data = TestAtomicData() | ||
|
||
plasma_species = [(carbon, 5, 2.e18, 800., Vector3D(0, 0, 0))] | ||
slab_length = 1.2 | ||
plasma = build_constant_slab_plasma(length=slab_length, width=1, height=1, electron_density=1e19, electron_temperature=1000., | ||
plasma_species=plasma_species, b_field=Vector3D(0, 10., 0)) | ||
plasma.atomic_data = atomic_data | ||
plasma.parent = world |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be put into a TestExcitationLine.setUp
method, to ensure the plasma state is consistent before each test and there is no retained state after each test. See https://docs.python.org/3/library/unittest.html#organizing-test-code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently the tests do not change the plasma state, but you are right, it is better to move this to the setUp()
method to avoid possible problems in the future. For all other tests I will do this in a separate PR.
cherab/openadas/rates/pec.pyx
Outdated
from cherab.core.utility.conversion import PhotonToJ | ||
|
||
|
||
DEF ZERO_THRESHOLD = 1.e-300 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be >0?
Also, prefer cdef ZERO_THRESHOLD
to DEF
: https://cython.readthedocs.io/en/latest/src/userguide/migrating_to_cy30.html#deprecation-of-def-if. Or just type the value out manually: it's only used in 3 places within the same function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initially I introduced this threshold when we switched to Raysect interpolators and moved to log-log interpolation for atomic rates. The idea was to not override the extrapolation settings of the interpolators by forcibly returning zero when extrapolation is disabled. However, if we decided to return zero rate for zero plasma parameters, it is safe to remove the threshold and just check if parameters are <= 0.
beam_energy = 50000 # keV | ||
beam_energy = 50000 # eV |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LOL, good catch.
# Install PECs for CVI spectral lines | ||
install_adf15(carbon, 5, 'adf15/pec96#c/pec96#c_pju#c5.dat', download=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel a bit uncomfortable about a demo modifying the users' atomic data repository. Should we in fact add this file to repository.populate()
? It's unclear why we don't have more carbon lines in there by default, given they parse fine with the existing parser? @Mateasek any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I think user database shouldn't be changed from a demo. It could lead to errors or worse, for example due to filesystem rights restrictions. If the file is provided by Open ADAS, I agree it should be rather added to the repository.populate()
. If it is not available yet, I would say comment out the file installation and add an additional comment for the users that they have to install it this way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, I added this to populate()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @vsnever for implemeting the changes. Once you sort out @jacklovell's comments, the PR can be accepted.
This draft PR fixes #57 by adding
ThermalCXLine
model and implementingThermalCXPEC
.ThermalCXPEC
is implemented as a function of 3 variables: electron density, electron temperature and donor temperature assuming Maxwellian distribution of both receiver and donor.PR currently misses:documentation,tests,demos.