Continue cmontecarlo tests refactor. Extend #530. #544

kdexd · 2016-04-19T02:49:22Z

This PR extends the work of #530. The tests which wrapped the C tests from test_cmontecarlo.so were redesigned to directly wrap C methods from cmontecarlo.so. A couple of TODOs commented in the file during the work of #530 have been resolved here.
The checklist states the tests which were added / refactored. The commit where the test was added is mentioned within parenthesis:

(No commit in parenthesis means the work on that method was done in #530.)

Not yet relevant:

These tests will have to be rewritten in future as the underlying code is experimental and at early development stage. Few of these tests have a @pytest.mark.skipif decorator too.

montecarlo_continuum_event_handler ( 870e5e4 )
montecarlo_free_free_scatter ( 4f4a1e8 )
montecarlo_bound_free_scatter ( 6e7be8c )
bf_cross_section ( 1f099af )
calculate_chi_bf ( 0de7cba )

After writing the above mentioned tests, following are undertaken as a routine to wrap up this task:

Every function in C has a test_<func> counterpart in python ( 02d1f13, acec56c )
Not yet implemented tests are marked skip with a descriptive reason ( ^ same )
Tests are grouped by 'importance', block comments separate the groups ( ^ same )
Removal of other test_*.c files ( 2b8465c ) and the building of test_cmontecarlo.so ( 70dcfac )

These methods are yet not tested. Refer #556 for details:

montecarlo_one_packet
montecarlo_one_packet_loop
montecarlo_main_loop
montecarlo_event_handler
macro_atom

kdexd · 2016-04-19T08:14:32Z

tardis/montecarlo/tests/test_cmontecarlo.py

+    ['packet_params', 'expected'],
+    [({'nu': 0.1, 'mu': 0.3, 'r': 7.5e14}, 0.0),
+     ({'nu': 0.2, 'mu': -.3, 'r': 7.7e14}, 0.0)]
+)


@wkerzendorf @yeganer There might be a problem in calculate_chi_bf's implementation, or I wonder chi_bf is zero for these test cases. I found a FIXME note in the underlying method, and I suspect that this method is indeed not thread-safe.

skip calculate_chi_bf

- Add a TODO as there is a problem in underlying method. Alongwith it also a skip marker is added.

kdexd · 2016-04-29T14:19:51Z

After #547 was merged, all the methods stopped throwing a segfault, except move_packet_across_shell_boundary. This is still troublesome.

yeganer · 2016-04-29T14:33:44Z

I don't think it's move_packet_accross_shell_boundary.

Is the list in the first post of this PR complete? If not, can you please add ALL functions which are in cmontecarlo.c, including those you already wrote tests for in the previous PR.

This gives us a nice overview of all functions and we can categorize them. For the moment I think the tests are mostly done because most of the functions left aren't relevant at the moment.

yeganer · 2016-04-29T22:02:18Z

- Add skip markers to two methods: test_montecarlo_bound_free_scatter test_montecarlo_free_free_scatter

- RKState struct in python implementation had a 'key' member which was a pointer to c_ulong type array. - The C counterpart had a static array declaration, and hence the memory allocated to both Structs was different, hence causing repeated segfaults, false assertions and fails.

wkerzendorf · 2016-05-03T19:45:08Z

@karandesai-96 there are several checkboxes not checked. You need to tell us if this PR is finished and what you want to do with the rest of the textboxes.

kdexd · 2016-05-03T20:41:14Z

@wkerzendorf These tests were classified in three different categories by @yeganer a few days back and I set my priorities accordingly. So here it goes:

Not yet relevant
- These are kept out of scope of this PR, whichever are left. Though only test for montecarlo_continuum_event_handler remains in this section. What is better thing to do- should I write it in this PR and add a @pytest.mark.skipif(True...) or tackle all of these when time arrives ?
Difficult to test
- I have been involved with these tests since quite a while now, and wish to switch over to some other part for a change. These four tests might consume time and delay this PR. So I am planning to open a new PR after doing one or two small tasks related to integration test. It would contain work for these tests.
Important
- I have worked on this PR assuming the tests falling under this category are the topmost priority. All the tests are written, and they pass, except test for macro_atom.
- macro_atom uses certain parameters of StorageModel for calculations, which are not set to default values in the fixture. For example: line2macro_level_upper is supposed to be an array - but I am unaware of its length, what decides its length ? What is the general range of numerical values which this parameter takes up ? There are a couple of them more.

I would need your opinion about the further work to do here. Should I list out the parameters which I need as a test case for macro_atom test so that we can form a suitable test case ? Rest all work in this PR is done, except the ones mentioned above are complete from my side unless I do modifications based on the reviews ;)

kdexd · 2016-05-03T20:45:42Z

Moreover, before merging we will make sure to remove test_cmontecarlo.c and test_montecarlo.so completely. These tests directly wrap around C methods and no longer rely on tests written in C.

wkerzendorf · 2016-05-03T21:31:08Z

@karandesai-96 finish this PR in the sense that you document

what it tests
list what you skip
why you skip it
open an issue where you list what needs to be done in another PR.

This needs to go in the description of the PR.

As it stands you are asking us to merge an unfinished PR. When you are done with something like this - the PR description should be cleaned up. @unoebauer's PRs are a very good example of how to do this. I should be able to go into a PR and figure out exactly what it does - what remains to be done, etc.

- New tests no longer require these C tests. They directly wrap C methods.

yeganer · 2016-05-04T07:33:47Z

Summary

Every function in C has a test_<func> counterpart in python
Not yet implemented tests are marked skip with a descriptive reason
Tests are grouped by 'importance', block comments separate the groups
Removal of all test_*.c files and the building of test_cmontecarlo.so
New Issue about missing tests for rpacket

Explanation

@karandesai-96 I had a look at the PR and I think it's mostly done. There is still some room for improvements.
For example I think it's a good idea to have a test_<method_name> for every function of the C-extension. For the "difficult to test" and "not important" categories they would be empty functions marked with pytest.mark.skip and a reason why this test should be skipped.

@wkerzendorf you can correct me if I'm wrong but in my opinion we should skip all functions listed as "not yet relevant", too. These functions are related to the continuum interactions which are developed by @chvogl. Running them for the current master doesn't seem necessary to me.

Finally I'd suggest you group the functions inside test_cmontecarlo.py by importance. This means all tests which are currently run should be at the top. Below them would be a block comment about the difficult to test methods and below that a block comment stating the next group isn't relevant because the implementation isn't final.

This PR aims to completely replace the tests for the C-extension. As such we should remove ALL files associated with testing the C part. I know there is test_rpacket.c which will be untested but there is only one relevant function (rpacket_init) and not testing that is safe for now. The removal of all files associated with the c-tests should be reflected by a new Issue about the newly untested methods.
You should remove the building of test_cmontecarlo.so, as it's not needed anymore.

- Tests are reordered according to category: - Important tests. - Not yet relevant tests. - Block comments have been added, which desribe these tests. - @pytest.mark.skipif added to 'Not yet relevant' category tests.

kdexd · 2016-05-04T19:56:02Z

Hi @yeganer I have done most of the work suggested. I have a doubt:

Removal of all test_*.c files and the building of test_cmontecarlo.so

test_rpacket.py wraps around this shared object. Should I remove that as well ? Or should I apply skip marker to all methods inside test_rpacket.py ?

yeganer · 2016-05-04T22:47:43Z

Remove test_rpacket.py
Last time I looked the file did nothing but testing the getters and setters. Since they are inlined we can't test them with ctypes. But they shouldn't break. If they break, then we'll know it because nothing works anymore.

kdexd · 2016-05-05T02:21:56Z

New Issue about missing tests for rpacket

@yeganer I have completed the points put up by you.

If the methods are static inline they aren't meant to be tested by python. Do you think opening an issue about that is a good idea ? I am anyways opening an issue for the requirement of difficult to test category.

wkerzendorf · 2016-05-09T09:26:04Z

@tardis-sn/tardis-core what is going on here? From my side this looks good.

wkerzendorf · 2016-05-10T13:16:59Z

@yeganer @unoebauer I'm signing off on this. Can one of you check and merge?

unoebauer · 2016-05-10T14:42:45Z

Reviewing the PR now...

unoebauer · 2016-05-10T14:47:52Z

Looks good - merging.

kdexd reviewed Apr 19, 2016
View reviewed changes

kdexd force-pushed the cmontecarlo-extend-pr-530 branch 5 times, most recently from 7f6750d to d996278 Compare April 20, 2016 03:23

kdexd mentioned this pull request Apr 22, 2016

Fix Issue #548 #547

Merged

karandesai-96 added 5 commits April 29, 2016 17:12

Refactor test_montecarlo_free_free_scatter.

4f4a1e8

Refactor test_bf_cross_section.

1f099af

Refactor test_move_packet.

e7a7b4e

Refactor test_calculate_chi_bf.

0de7cba

- Add a TODO as there is a problem in underlying method. Alongwith it also a skip marker is added.

Refactor test_montecarlo_bound_free_scatter.

6e7be8c

kdexd force-pushed the cmontecarlo-extend-pr-530 branch from c283397 to d6b4b4e Compare April 29, 2016 13:57

kdexd force-pushed the cmontecarlo-extend-pr-530 branch from d6b4b4e to 6e7be8c Compare April 29, 2016 19:23

Add test_reverse_binary_search and parametrize it.

50cf9f0

- Add skip markers to two methods: test_montecarlo_bound_free_scatter test_montecarlo_free_free_scatter

kdexd force-pushed the cmontecarlo-extend-pr-530 branch from 901690b to 50cf9f0 Compare April 30, 2016 18:40

karandesai-96 added 5 commits May 1, 2016 08:45

Add test_line_search and parametrize it.

5441c00

Refactor, parametrize test_montecarlo_thomson_scatter.

a776364

Add default value of chi_cont in packet fixture.

5fcf136

Refactor, parametrize test_move_packet_across_shell_boundary.

fd9d4c1

karandesai-96 added 2 commits May 4, 2016 10:32

Add test_montecarlo_continuum_event_handler [unclear].

870e5e4

Remove test_cmontecarlo.c

70dcfac

- New tests no longer require these C tests. They directly wrap C methods.

karandesai-96 added 2 commits May 5, 2016 00:31

Reorder tests for categorization, add block comments.

02d1f13

- Tests are reordered according to category: - Important tests. - Not yet relevant tests. - Block comments have been added, which desribe these tests. - @pytest.mark.skipif added to 'Not yet relevant' category tests.

Add block comment and remaining incomplete tests.

acec56c

kdexd force-pushed the cmontecarlo-extend-pr-530 branch from f356cfd to acec56c Compare May 4, 2016 19:43

Remove test_rpacket.c and test_rpacket.py

2b8465c

kdexd mentioned this pull request May 5, 2016

test_cmontecarlo.py: partial coverage of cmontecarlo.c #556

Closed

Remove code related to building test_montecarlo.so

51cbe16

unoebauer merged commit 95b117d into tardis-sn:master May 10, 2016

kdexd deleted the cmontecarlo-extend-pr-530 branch May 10, 2016 16:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Continue cmontecarlo tests refactor. Extend #530. #544

Continue cmontecarlo tests refactor. Extend #530. #544

kdexd commented Apr 19, 2016 •

edited

Loading

kdexd Apr 19, 2016 •

edited

Loading

yeganer Apr 19, 2016

kdexd commented Apr 29, 2016

yeganer commented Apr 29, 2016

yeganer commented Apr 29, 2016 •

edited

Loading

wkerzendorf commented May 3, 2016

kdexd commented May 3, 2016 •

edited

Loading

kdexd commented May 3, 2016 •

edited

Loading

wkerzendorf commented May 3, 2016 •

edited

Loading

yeganer commented May 4, 2016 •

edited

Loading

kdexd commented May 4, 2016 •

edited

Loading

yeganer commented May 4, 2016

kdexd commented May 5, 2016 •

edited

Loading

wkerzendorf commented May 9, 2016

wkerzendorf commented May 10, 2016

unoebauer commented May 10, 2016

unoebauer commented May 10, 2016

Continue cmontecarlo tests refactor. Extend #530. #544

Continue cmontecarlo tests refactor. Extend #530. #544

Conversation

kdexd commented Apr 19, 2016 • edited Loading

Not yet relevant:

kdexd Apr 19, 2016 • edited Loading

Choose a reason for hiding this comment

yeganer Apr 19, 2016

Choose a reason for hiding this comment

kdexd commented Apr 29, 2016

yeganer commented Apr 29, 2016

yeganer commented Apr 29, 2016 • edited Loading

Important

Difficult to test

Not yet relevant

wkerzendorf commented May 3, 2016

kdexd commented May 3, 2016 • edited Loading

kdexd commented May 3, 2016 • edited Loading

wkerzendorf commented May 3, 2016 • edited Loading

yeganer commented May 4, 2016 • edited Loading

Summary

Explanation

kdexd commented May 4, 2016 • edited Loading

yeganer commented May 4, 2016

kdexd commented May 5, 2016 • edited Loading

wkerzendorf commented May 9, 2016

wkerzendorf commented May 10, 2016

unoebauer commented May 10, 2016

unoebauer commented May 10, 2016

kdexd commented Apr 19, 2016 •

edited

Loading

kdexd Apr 19, 2016 •

edited

Loading

yeganer commented Apr 29, 2016 •

edited

Loading

kdexd commented May 3, 2016 •

edited

Loading

kdexd commented May 3, 2016 •

edited

Loading

wkerzendorf commented May 3, 2016 •

edited

Loading

yeganer commented May 4, 2016 •

edited

Loading

kdexd commented May 4, 2016 •

edited

Loading

kdexd commented May 5, 2016 •

edited

Loading