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

Use sigma_T from astropy constants instead of a hardcoded value #688

Closed
wants to merge 4 commits into from

Conversation

ftsamis
Copy link
Member

@ftsamis ftsamis commented Jan 10, 2017

Astropy now has a constant defined for sigma_thomson. This PR replaces the hardcoded value with it.

Props to @chvogl for discovering this.

For tardis_example outputs before and after this patch see: https://gist.github.com/ftsamis/eccbcc1b2a29053a412a2d74e5a11e7f

Copy link
Contributor

@yeganer yeganer left a comment

Choose a reason for hiding this comment

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

Please update the reference date files.

@ftsamis
Copy link
Member Author

ftsamis commented Jan 15, 2017

@yeganer Not sure what you are referring to. Do you mean the test_cmontecarlo module? If so, done.

@yeganer
Copy link
Contributor

yeganer commented Jan 15, 2017

No, I mean test_data.h5 and the numpy reference data files for test_tardis_full.py

@wkerzendorf
Copy link
Member

@ftsamis @chvogl I like this change. Can we get this into a mergeable state?

@chvogl
Copy link
Contributor

chvogl commented May 22, 2017

@wkerzendorf As @yeganer pointed out someone would have to update the reference data files (test_data.h5 and the numpy reference data files for test_tardis_full.py) before we can merge this.

@unoebauer
Copy link
Contributor

I'd still like to merge this. Let's resolve the conflicts and update the reference data.

@unoebauer unoebauer added this to the v2.0 milestone Oct 9, 2017
@unoebauer unoebauer modified the milestones: v2.0, beyond-2.0 Apr 5, 2018
@unoebauer
Copy link
Contributor

unoebauer commented Aug 16, 2018

I am a bit puzzled why some of the test differ by so much since the change in the value of the Thomson cross section is really minuscule:

Before we used the hard-coded value of

6.652486e-25 

Now, we rely on astropy.constants, which includes a value for the Thomson cross section of

6.652458734e-25

in version 1.3.3.

This is a change on the level of 1e-6. Yet, in some tests, the differences to the reference values are on the per cent level.

Any thoughts on this, @wkerzendorf, @chvogl?

@unoebauer
Copy link
Contributor

Closing. Work continues in PR #863

@unoebauer unoebauer closed this Aug 17, 2018
wkerzendorf pushed a commit that referenced this pull request Aug 19, 2018
* Use sigma_T from astropy constants instead of hardcoding it, also fixing its unit.

* Use sigma_T constant in test_cmontecarlo instead of a hardcoded value.

* Change refdata branch

* Update .travis.yml
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.

5 participants