Fix energy conservation #495
Conversation
* whenever a packet Thomson scatters or performs a line scattering, the Doppler factor at the interaction location is used (instead of the Doppler factor at the beginning of the trajectory segment)
|
@unoebauer When I first looked at the C code I was wondering why move_packed returned the doppler_factor. My suggestion: add |
|
Fix does not seem to induce significant changes for standard setups:
|
|
@yeganer, @ssim, @wkerzendorf, @chvogl Many tests had to be updated - for future references: we should write a script which recalculates all the data files once critical changes have been made within the Monte Carlo routine. Of course, before using these files, they should be carefully compared with the current ones. I have started a very crude version of such a script. |
|
@unoebauer Last time we did this I advised against creating such a script because it could be used carelessly. But I guess with future slow tests coming we might need that. |
|
Well - true, there is a temptation to use it carelessly. But manually building all the data files is really painful! |
|
Ok - Travis is finally happy. Should this be merged @wkerzendorf, @yeganer? It would be good if you, @ssim and @chvogl, could also have a careful look at it. Before merging, I'll clean-up the commit history. |
* Move packet no longer returns the Doppler factor since this functionality is no longer required * temporarily deactivated test_move_packet in test_cmontecarlo.c * marked it skipped due to bad design
* update reference data for tardis_full_test * update reference data for simulation_test * update reference data for plasma_full_test
| dealloc_storage_model(&sm); | ||
| return res; | ||
| } | ||
| //double |
There was a problem hiding this comment.
@unoebauer should we just delete these lines?
There was a problem hiding this comment.
Well - I wouldn't for now. Eventually, we want to test move_packet, just in a different way. I'd keep these lines as a reminder.
unoebauer
force-pushed
the
energy_conservation_fix
branch
from
587bde3
to
95ca67c
Compare
|
I think this is ready to get merged. Regarding |
|
I think that yes, I agree with this fix. I wonder how it came about that there was an error here but I think that yes, it is was wrong. It would have been correct if the Dopper factor were re-calculated at the end of "move packet" before being returned, but that was not being done. This seems like the right fix to me. |
Fix energy conservation
As found by @chvogl and reported in issue #448, an inconsistency in the energy conservation calculation exists. Instead of the Doppler factor at the interaction location, the initial Doppler factor, determined at the beginning of the trajectory segment is used. For typical applications, the consequences of this inconsistency should be small (as discussed in #448). Nevertheless, this PR aims at fixing this inconsistency.