Skip to content
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

Fix NLTE Normalization - issue #784 #831

Merged
merged 5 commits into from
Apr 29, 2019

Conversation

livnehra
Copy link
Contributor

Fix NLTE normalization according to @ssim suggestion in the discussion in #784

@@ -190,7 +190,7 @@ def _main_nlte_calculation(
else:
raise e
general_level_boltzmann_factor[i].ix[species] = \
level_boltzmann_factor
level_boltzmann_factor * g[species][0] / level_boltzmann_factor[0]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@livnehra can you make this g.loc[species, 0]

@@ -190,7 +190,7 @@ def _main_nlte_calculation(
else:
raise e
general_level_boltzmann_factor[i].ix[species] = \
level_boltzmann_factor * g[species][0] / level_boltzmann_factor[0]
level_boltzmann_factor * g.loc[species][0] / level_boltzmann_factor[0]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.loc[species, 0]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That doesn't work...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm - that's interesting. I'll check it tomorrow.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes you're right - I screwed things up in my mind. I think the way you do it now is good.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember, for example species=(14,1) for Si II

@wkerzendorf
Copy link
Member

@livnehra the tests fail - your change somehow breaks the code - have a look at the continuous integration to see what's wrong.

@livnehra
Copy link
Contributor Author

Hopefully should pass the tests now.

@livnehra
Copy link
Contributor Author

So the fails now are due to NLTE results changing, which makes sense if the previous results were obtained with the erroneous g_ratio_matrices.

@wkerzendorf
Copy link
Member

@livnehra - okay I'm ready to update the reference data. I still think it would be good to get a spectrum beforehand.

@unoebauer
Copy link
Contributor

@wkerzendorf, @livnehra - what's the idea about proceeding with this PR?

@wkerzendorf
Copy link
Member

@livnehra - we haven't forgotten about you. We need regenerate the reference data and then we'll merge.

@@ -442,7 +442,7 @@ def _create_collision_coefficient_matrix(self):
C_ul_matrix[level_number_lower, level_number_upper, :] = line.values[2:]
delta_E_matrix[level_number_lower, level_number_upper] = line['delta_e']
#TODO TARDISATOMIC fix change the g_ratio to be the otherway round - I flip them now here.
Copy link
Contributor

@chvogl chvogl Aug 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be possible to remove the TODO.

@wkerzendorf wkerzendorf merged commit dff362e into tardis-sn:master Apr 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants