-
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
Improve density and direction calculation in the neutral beam model #433
Improve density and direction calculation in the neutral beam model #433
Conversation
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,
thanks for the changes. The code itself looks clean and I couldn't find any problems. I would only suggest one improvement regarding the documentation. Since the beam direction became more complex, I think it would be nice to add the equations you use into the documentation. I think it would be enough if you add the equations and plot you posted in the opening post of this PR. It would be explanatory enough and would remove the necessity to go through the cython code for the users, what do you think?
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. I'm with @Mateasek: the changes are sound but it would be even better to explicitly document what calculation is being used.
I'd also suggest adding some emphasis (**
or *
) to the line in the changelog highlighting that this will change existing results.
If there aren't already any unit test associated with this, now is also a good time to add them. But don't worry if that's too big an project: I wouldn't want to hold up an otherwise useful change because of the work involved in setting up a mock beam for tests.
…gleRayAttenuator from CXS models section to Beam Attenuation section.
Thanks for the reviews, @Mateasek, @jacklovell.
I added equations for the beam density and direction in
I agree, the line announcing this change in the changelog is now in bold.
We already have tests for the beam CX model, so setting up a mock beam and plasma is not a problem. I added the tests for the beam density and direction. They test these values on-axis, off-axis and outside the beam. |
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 minor suggestions.
Thanks, @jacklovell. All done. |
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 improving the docstrings, I have no further suggestions.
This updates density and direction calculation of the neutral beam model and fixes #414.
The broadening of the BES line shape is now naturally determined by the beam divergence. Therefore, setting beam temperature to the physically unreasonable values is no longer required. The parameters of the beam in the BES demo are updated.
Although this change does impact user results, I think it is justified by the improved consistency of the beam model.