-
-
Notifications
You must be signed in to change notification settings - Fork 410
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
NLTE #347
NLTE #347
Conversation
@wkerzendorf So I think I've got a good system here with the NLTE now. Could you have a look and see what you think? It works on a similar principle to the NLTE in the old plasma. Quick summary: So the property cycling issue consists of two property cycles. The NLTE calculation indirectly affects the Old plasma calculation sequence: I hope that makes some sense! If you're happy with this, I'd like to merge it although the coverage test has failed. I plan to open a new PR for adding the tests. |
@aoifeboyle do they converge to the same values? |
@wkerzendorf Yep! Better accuracy than I expected in fact. |
@@ -0,0 +1,17 @@ | |||
from tardis.plasma.properties.base import BasePlasmaProperty |
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.
@wkerzendorf Think I've found a better way of doing this now, without having to output and read in values from previous iterations from model.py. Created this new property class to store values from previous iterations. The values are set just prior to all of the properties being updated. I think it's a lot more straightforward! Let me know if you think I'm on the right track now and then I'll tidy it up/finish it off.
return general_level_boltzmann_factor | ||
def _main_nlte_calculation(self): | ||
pass | ||
''' |
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 @wkerzendorf. Just trying to get the basic structure here to work like you suggested earlier (lines 208-230). When I run this I get the error, 'Can't instantiate abstract class LevelBoltzmannFactorNLTE with abstract methods calculate.' I assume that's just because there's no explicit def calculate()
? How do I make this work?
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.
you define one:
def calculate(self, *args, **kwargs):
pass
then overwrite it
self.calculate = self._calculate_coronal_approximation | ||
else: | ||
self.calculate = self._calculate_general | ||
self._update_inputs() |
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.
that is smart!
New approach to NLTE. This time the
beta_sobolevs
from the previous iteration of the plasma are used to calculate the NLTE level populations, in a similar way to what was done in the old plasma. Seems to be working well, but I'm a bit confused about the atomic data, specifically whether thelines_idx
in the NLTE data should be the same aslines.index
in the atomic data. It seems like that should be the case for consistency, but it doesn't seem to be the case here, and it's creating some confusion when calculating the NLTE level populations. Here,lines_index
is taken from the NLTE data: