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

improve inlining specifications and fix a few problems #386

Merged
merged 4 commits into from
Aug 21, 2015
Merged

improve inlining specifications and fix a few problems #386

merged 4 commits into from
Aug 21, 2015

Conversation

mreineck
Copy link
Contributor

This pull request should enable all small rpacket-related functions to be inlinable, by moving them from rpacket.c to rpacket.h. I also fixed a few other inline-related problems, added missing prototypes to the header files and created an extra header for mt_state and line_search(), to avoid recursive header inclusion.

@wkerzendorf
Copy link
Member

@mreineck this is great!! let's see if it passes the tests.

@wkerzendorf
Copy link
Member

@mreineck Okay it seems to be stumbling at rpacket_get_status at https://github.com/tardis-sn/tardis/blob/master/tardis/montecarlo/src/test_cmontecarlo.c#L274. I think the status is an enum but maybe I'm wrong.

@wkerzendorf
Copy link
Member

@mreineck there is always the possibility to mark this test as xfail - expected to fail.

@unoebauer
Copy link
Contributor

@mreineck, @wkerzendorf
I can confirm the speed-up of about a factor 2 when using the tardis_example setup
Tardis Master:
Running with OpenMP - 1 threadstardis.simulation - INFO - Finished in 20 iterations and took 95.94 s
PR 386:
Running with OpenMP - 1 threadstardis.simulation - INFO - Finished in 20 iterations and took 55.12 s

@wkerzendorf
Copy link
Member

@unoebauer this is very good!!

@wkerzendorf
Copy link
Member

I think we are now roughly a factor of 10 below my initial cython implementation 😉

@unoebauer
Copy link
Contributor

@mreinecke @wkerzendorf
The virtual spectra obtain with Tardis Master and PR 386 are also identical

@@ -274,7 +278,7 @@ bool
test_montecarlo_bound_free_scatter(){
double DISTANCE = 1e13;
montecarlo_bound_free_scatter(rp, sm, DISTANCE);
return rpacket_get_status(rp);
return (rpacket_get_status(rp)!=0);
Copy link
Member

Choose a reason for hiding this comment

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

@mreineck no mark it as xfail in the python file. Here's how: https://github.com/tardis-sn/tardis/blob/master/tardis/montecarlo/tests/test_cmontecarlo.py#L74 add @pytest.mark.xfail above that. that will show us continually that this is an xfail.

@mreineck
Copy link
Contributor Author

I did that now as well. But the condition in the C code was also incorrect: thet status can go from 0 to some positive value>1, but the return type of bool indicates that only 0 and 1 are acceptable.

@wkerzendorf
Copy link
Member

Well it seems all checks pass. @orbitfold can you have a quick view as well? If we are all happy - let's merge.

orbitfold added a commit that referenced this pull request Aug 21, 2015
improve inlining specifications and fix a few problems
@orbitfold orbitfold merged commit 8bdc108 into tardis-sn:master Aug 21, 2015
@mreineck mreineck deleted the c_tweaks branch August 31, 2015 10:19
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