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

Updates to He NLTE treatment. #542

Merged
merged 9 commits into from
Jul 11, 2016
Merged

Conversation

aoifeboyle
Copy link
Contributor

In the process of ensuring that the results for my paper were reproducible with the new plasma, I found some discrepancies and made some changes. I think there are parts of this that could be better coded but at least it works. I will try to improve/condense it in a while. This now gives the correct results.

Aoife

@wkerzendorf
Copy link
Member

@aoifeboyle is this the new code that is used to make the paper. Why doesn't it pass?

@aoifeboyle
Copy link
Contributor Author

Hi @wkerzendorf. Sorry, busy with other stuff. Will add this to the to-do list but might need to wait til the weekend. I changed the ion_number_density inputs so that the helium ion populations could be calculated correctly in that property so now I need to reorganise property_collections. There may be a cleaner way to do this so I need to think about it for a while.

@aoifeboyle
Copy link
Contributor Author

@wkerzendorf How is this as far as removing the duplicate code?

@wkerzendorf
Copy link
Member

it's still showing duplicates. but we can look at that later.

@aoifeboyle
Copy link
Contributor Author

@wkerzendorf Does that mean we can merge?

@wkerzendorf
Copy link
Member

@aoifeboyle We should get rid of them - if we sit together this should not take much longer than an hour. 13.30 today?

"""
Calculates the He III level population values.
"""
zeta = PhiSahaNebular.get_zeta_values(zeta_data, 2, t_rad)[1]
he_three_population = (2 / electron_densities) * \
he_three_population = 2 * \
Copy link
Contributor

Choose a reason for hiding this comment

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

You can wrap the whole equation in () then you don't need \ at the end of line which should be avoided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, sure. What's the difference?

Copy link
Contributor

Choose a reason for hiding this comment

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

To me \ at the end of a line looks ugly and PEP recommends to use ( ) instead.
The added benefit of the 2nd approach is that most editors will indent the following lines differently which looks nicer, too.

@aoifeboyle
Copy link
Contributor Author

@wkerzendorf Sorry, only saw this yesterday evening. I'm focusing on the draft right now so I think it will have to wait til next week.

@wkerzendorf wkerzendorf merged commit 0be20d1 into tardis-sn:master Jul 11, 2016
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.

3 participants