search

next-exp / nexus

Geant4 simulation framework of the NEXT Collaboration
5 stars 55 forks source link

Fix Xe density #242

Closed enakshidey closed 4 months ago

enakshidey commented 6 months ago

In the Material.cc file the calculation of the Xe density did not take into account the isotopic composition of Xe for different Xe types (e.g depleted, enriched, natural). The density was only function of pressure and directly taken from the NIST data table. But percentage contribution from different isotopes should also be included for different Xe types. So I have modified the density calculation including the isotopic composition.

paolafer commented 6 months ago

Hi @enakshidey, is the PR ready for review or do you still need some work to do? I notice that the code as it is isn't compiling.

paolafer commented 6 months ago

Also, please, have a look at the NEXUS coding convention and try to follow them in the code style.

enakshidey commented 6 months ago

Hi @paolafer I still need to modify few things there. It's not yet ready.

enakshidey commented 5 months ago

IsotopicComposition Previously in the Materials.cc code it was not taking in to account the isotopic masses of various Xe isotopes. So the density calculation was incorrect. I have included the isotopic masses in the density calculation. And in the plot we can see the blob energy does not depend on what gas you put at a particular pressure.

paolafer commented 5 months ago

Can you please rebase your branch on the master? For some reason, it doesn't compile on my laptop, although it does run on the github action.

paolafer commented 5 months ago

I'm afraid I'm a bit lost with the units over the code. Does the user need to provide pressure and temperature in specific units or not? Could you add a test to ensure that the calculations are done correctly?

paolafer commented 5 months ago

Hi @enakshidey! I've talked to the other reviewer (@gonzaponte) and we agreed on the following suggestions (in case you found our individual comments confusing - which they are, sorry for that!).

There's no need of changing the units within the functions, as long as all the variables have the appropriate ones. Therefore, you just need to define const double R = 8.314 * joule / (mole*kelvin); in the CalculateGasDensityFromIsotopicComposition() function and

double T_r = temperature/(289.733*kelvin); double P_r = pressure/(58.420*bar);

in the CompressibilityFactor() function. Once you have that, the density can be just defined as double density = (pressure * average_molar_mass) / (z * R * temperature);. Do you agree?

Also, we would define directly isotopicCompisition as 124, 0.095 * perCent. This way, you can drop the division by 100 in line 43 and the multiplication by perCent whenever you add an isotope to the material.

Notice that the tests are failing with the actual version of the code. I'm not exactly sure why, but the problem seems to be that there's no light reaching both SiPMs and PMTs in the events generated in the tests, which may be caused by a wrong implementation of the density.

enakshidey commented 5 months ago

Thank you both @paolafer @gonzaponte for the inputs. I know why the test failed. I was checking the density value in normal unit kg/m^3 to make sense if everything is correct and forgot to change it in the G4units back. But you are right I have done few unnecessary unit conversion. I will remove those things as you have mentioned and submit the modified code as soon as possible.

And extremely sorry for all these inconveniences.

paolafer commented 4 months ago

Good job, @enakshidey! There are a few more style-related comments, after which I would approve this PR.