WIP: Helium approx. #374
WIP: Helium approx. #374
Conversation
|
@wkerzendorf Could you take a look at this and see if you're happy with how it's coded? I still have to add tests etc. |
| helium_population = deepcopy(level_boltzmann_factor.ix[2]) | ||
| # He I excited states | ||
| he_one_population = level_boltzmann_factor.ix[2].ix[0] * \ |
There was a problem hiding this comment.
this calculation is done somewhere else as well. Let's just make one function for it.
There was a problem hiding this comment.
@wkerzendorf So the He I level population calculation here is different to what the code normally uses because it's calculated from the GS of the He II ion rather than the He I ion. You're right with the He II and He III level populations though, but it's easier for me to keep all of these calculations in the same function so that I can normalise the populations easily using the number density. But I'm having a think now about ways to simplify it.
There was a problem hiding this comment.
Ok so I was thinking that I could just use some kind of scaling method to change the He II and He III populations after the He II ground state population is set to 1.0, but that would require using the level_number_density property as an input property here, which would create a loop. I think this is the only way to do it simply.
There was a problem hiding this comment.
@aoifeboyle What I meant was. Add functions (maybe static ones) to the other properties in the code that do similar calculations and then both properties use their functions. One such example is in level_populations in line 26 and 47. they both use the same calculation, but it is in the code twice. Use a static function there. We can skype on this if you want.
There was a problem hiding this comment.
@wkerzendorf Ok, so I can leave the HeliumNLTE property alone? I'll merge the two level_population properties into one now.
There was a problem hiding this comment.
@wkerzendorf Let me know if you think what I have now for the level_populations is better. I had another look and all of the level population calculations in the HeliumNLTE property are different to the level population calculations in LevelNumberDensity because they are all calculated relative to ground state populations rather than using the ion populations. So being able to call the same calculations would not be helpful. Let me know if that makes sense to you. I think it's tidier now anyway.
|
@aoifeboyle looks like a good first effort, but I feel that we should make sure that code is not repeated. Let me know if that explains it. |
|
@wkerzendorf Ok, I see what you mean. Leave it with me. |
| ionization_data, beta_rad, g, g_electron, w, t_rad, t_electrons, | ||
| delta, zeta_data, number_density): | ||
| helium_population = deepcopy(level_boltzmann_factor.ix[2]) |
There was a problem hiding this comment.
Is there a reason to use deepcopy? It can be incredibly slow, could be a problem if this is called often.
There was a problem hiding this comment.
@orbitfold Not really! I just use it from habit. I didn't know it was slow. I'll replace it with copy.
There was a problem hiding this comment.
Well you might need it depending on what you want to do. But if you can use copy do that.
|
hi @aoifeboyle I can't see your comments for some reason on github (am getting emails about them). I can't look at it now in detail. But while we wait - have a look at line 26 and 47 in level_populations. you can make it one function callable from the other one (use static methods - let me know if that's unclear). |
Merge branch 'master' of github.com:tardis-sn/tardis into plasma/helium
|
@wkerzendorf So I've changed the Helium NLTE stuff so it makes use of the functions that already exist as static methods (the Phis, etc.), and I've removed repetition from other parts of the code too. Could you have a look through and tell me what you think when you have time? |
| phis = (1 / PhiSahaLTE.calculate(g_electron, beta_rad, | ||
| partition_function, ionization_data)) * electron_densities * \ | ||
| (1.0/g.ix[2].ix[1].ix[0]) * (1/w) * (t_rad/t_electron)**(0.5) |
There was a problem hiding this comment.
you can do .ix[2, 1, 0]. Maybe try not to us ix but iloc.
There was a problem hiding this comment.
Changed to .ix[2,1,0]. I used to use .iloc but it doesn't work with one of the Pandas versions on here so I always use .ix now.
|
@wkerzendorf Can I merge this? |
|
@wkerzendorf Got the error 245 for one of the Travis checks here but nothing happens when I click the button to re-run it for some reason? It's definitely not the code because the previous commit worked fine and this one was just a small change to an .rst file. |
|
@aoifeboyle I think that looks very good. As I'm quite busy this week, I'll let @orbitfold have another look for any code style issues and then we can merge (do this at your own discretion). |
|
Well, one obvious thing is that there are no docstrings. And the chances of anyone adding them later if you don't now are slim :) And there should really be docstrings for each method explaining the arguments and what the method does and what it returns. I don't believe we are very consistent with our docstring conventions but any docstring is better than none. This is as good as anything for a style guide I guess: http://sphinxcontrib-napoleon.readthedocs.org/en/latest/example_google.html Or look at our code that does contain them. |
|
We use numpydoc docstring format (if we do)
|
|
Ok, I'll start on that now! |
|
@orbitfold Let me know if there are any other issues, because I might merge this and then create a new PR with docstrings for the whole plasma. |
|
I just looked through it and I can't think of anything nasty to say about it, I think it's fine. Why do you need a separate PR for docstrings though? |
|
I don't have to. It's just I haven't done proper docstrings for all the other plasma stuff I've added, so I'd like to do the whole lot at once. |
|
Either way is fine, just curious :) |
Still working on this.