Move particle opacity filling into FillParticleOpacities - #138
erickurquilla wants to merge 5 commits into
Conversation
…ttering work can merge these files cleanly. The new files match erick/scattering; interpolate_rhs_from_mesh now calls them with flags that preserve current development behavior. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Is this adding new functionality or just reorganizing into another file? Could we do a PR with the new file organization, since otherwise it will hide a large number of changes? |
|
Not adding new functionality, just reorganizing and moving code. |
|
do you mean "not adding new functionality"? |
|
Yes! I mean, no new functionality |
|
It looks like some of the code changed - for one thing there is no more "... fix it ..." commented. This is obviously not functional in any way, but I worry it means the functionality did change. |
|
Merged developments into this branch. Conflicts were resolved. Feel free to review. |
srichers
left a comment
srichers
left a comment
There was a problem hiding this comment.
I left a few small comments, but I have a general overarching question. It looks like this PR is centered around some kind of optimization: the function signature (i.e. the inputs and outputs) are the same no matter what options are passed in, but it looks like the idea is to not compute things that are not needed. Does this improve the speed of any of your calculations in a measurable way?
| amrex::Real munu[NUM_FLAVORS][NUM_FLAVORS], | ||
| amrex::Real munubar[NUM_FLAVORS][NUM_FLAVORS]) { | ||
| void fill_particle_opacities( | ||
| const TestParams* parms, amrex::Real rho_pp, amrex::Real T_pp, |
There was a problem hiding this comment.
would this be better passed by reference? I think right now it says that the pointer to params can't change, but the values within the object pointed to can.
There was a problem hiding this comment.
I think it is the opposite. const TestParams* parms already protects the object, not the pointer. This is nevertheless the standard way parms is called in pretty much all EMU functions.
My agent recommendation is: I would still keep the pointer. TestParams is Gpu::Managed, lives behind unique_ptr in main.cpp, and is threaded through the whole GPU path as const TestParams* so device lambdas capture a managed-memory address by value. fill_particle_opacities is AMREX_GPU_HOST_DEVICE and is called from that MeshToParticle kernel with the captured pointer. Changing only this helper would be inconsistent with every other parms argument, and capturing a host reference into a device lambda is a classic way to get a host stack address on the GPU.
There was a problem hiding this comment.
ah, both methods protect the data. Const on the other side of the variable name would protect the pointer value.
| //-------------------- Values from EoS table ------------------------------ | ||
| if (interpolate_chemical_potentials == 1) { | ||
| double mue_out, | ||
| muhat_out; // mue_out : Electron chemical potential. muhat_out : neutron minus proton chemical potential |
There was a problem hiding this comment.
I think many of these comments would look cleaner if you write the comment on the line above so it doesn't get broken into many lines.
| if (parms->IMFP_method == 0) { | ||
| // do nothing | ||
| } else if (parms->IMFP_method == 1) { | ||
| Real scat_diag[NUM_FLAVORS]; |
There was a problem hiding this comment.
Declare these above, since it's used in multiple places. It shouldn't change anything about the speed, since the variable will be created in the stack no matter which branch is used.
|
Sorry, I did not explain it. I do not think these changes are "centered around some kind of optimization" or driven by a desire to make the code faster. They mainly involve moving code from one place to another and, more importantly, creating a function other places can call. Later, when we implement scattering, it will become clear that we need to call this function in multiple places. I did this because we probably don't want duplicate code, and we also want to keep the code clean by avoiding unnecessary variables. This new function can, for example, return only bracket scattering opacities or only absorption opacities if needed. The output quantities depend on the flags provided. Even when the input is the same, you can set an unused variable to a null pointer for variables that won't be computed. This code is cleaner because it avoids defining unused variables. The use isn't evident now, but I hope it becomes evident later. PD: I am writing this while waiting for my flight to El Salvador. I do not work at this time, lol. |
|
I went through the comments and made some changes. Feel free to review again. |
|
Thanks for your hard work! I think there is still more to discuss - I don't think this makes the code "cleaner" by not returning unused variables - the code for those variables still exists, and it takes more lines of code to select which variables to return than it does to just return all of them. So there are two potential paths here, and I think they both have the same resolution, which is a bit different from what is done here. Optimization: one does not want to unnecessarily calculate values that are not used, since this takes extra time. Of course, doing fewer calculations makes the code faster. Cleanliness/organization: a function should do the thing it is supposed to do, and not a bunch of unrelated work. In both cases, I think the way the code should be organized is to break it into functions that do the specified task. So there would be one function that calculates the equilibrium distribution based on the chemical potential. Then another function that calculates the scattering opacitiy matrix. Then another that calculates the absorption opacity matrix. Then, rather than passing flags to the function depending on where in the code it is, we just call the appropriate function in the appropriate place. This would make the code more modular (also a good thing), make it more readable, and remove if statements and unneeded variables. |
|
Thanks for the detailed explanation. That makes sense, and I agree this is a better approach than what I originally did. Here's what I'll change based on your comment:
This should address both the performance concern (no unnecessary calculations) and the readability/modularity concern (each function has a single, clear responsibility). Let me know if this matches what you had in mind. I'll go ahead and make these changes. |
2 participants
Summary
interpolate_rhs_from_meshintoFillParticleOpacities.H/.cpp, matchingerick/scatteringbyte-for-byte so those files can merge later without conflicts.Make.packageand callfill_particle_opacitieswith flags that preserve current development behavior (absorption + chemical potentials always; scattering diagonals only for IMFP_method 2; no brackets).Test plan
git diff erick/scattering -- Source/FillParticleOpacities.H Source/FillParticleOpacities.cppis emptyerick/scatteringconflicts only inEvolve.cpp, not inFillParticleOpacities.H/.cppMade with Cursor