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

[GSoC 2015 WIP] Testing other montecarlo functions #330

Merged
merged 9 commits into from
Aug 3, 2015
Merged

[GSoC 2015 WIP] Testing other montecarlo functions #330

merged 9 commits into from
Aug 3, 2015

Conversation

kaushik94
Copy link
Contributor

@wkerzendorf is this how it should be done ?

@kaushik94 kaushik94 mentioned this pull request Jun 15, 2015
20 tasks
@kaushik94
Copy link
Contributor Author

@wkerzendorf how should we go about testing the other functions that don't return anything ?

@wkerzendorf
Copy link
Member

can you give an example?

@kaushik94
Copy link
Contributor Author

montecarlo_line_scatter ?

@wkerzendorf
Copy link
Member

@kaushik94 that is a function that does the scatter of the montecarlo particles. What will change is the rpacket. This is some physics - can you make a PR just testing the montecarlo_line_scatter. @aoifeboyle can you help with the physics here.

@kaushik94
Copy link
Contributor Author

@wkerzendorf oh, In that case I guess all the functions in #295 need to be tested the same way. The trivial functions are tested in this PR. Can this be merged now ? And regarding the rest of the functions, I might need help from @aoifeboyle . @aoifeboyle please let me know a good time for you tomorrow, we can finish all of them off tomorrow.

@aoifeboyle
Copy link
Contributor

@wkerzendorf @kaushik94 I'm not very clear on the montecarlo stuff but I can definitely try! Give me some time to have a look through it here. So should I just devise suitable tests for all of the functions?

@wkerzendorf
Copy link
Member

@aoifeboyle - no specifically have a look at montecarlo_line_scatter. @kaushik94 will open a new PR on this soon and then we can think about what physics makes sense to test.

@aoifeboyle
Copy link
Contributor

@wkerzendorf Ok, having a look through it now!

}

double
test_compute_distance2electron(){
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would like to test these functions. What should I start testing with ? Can anyone help me with simulating a decent rpacket and a storage model ? @aoifeboyle or @wkerzendorf .

Copy link
Contributor

Choose a reason for hiding this comment

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

It's quite late here now, but I can help tomorrow @kaushik94 :)

@aoifeboyle
Copy link
Contributor

@kaushik94, I think I'll start by creating a test for the montecarlo_line_scatter function as you were planning to. I'm not that clear on C so I think that would be a good exercise to help me understand it. And I'll create an rpacket and storage_model that makes sense in terms of the physics to test it with. Does that sound ok for starters?

@kaushik94
Copy link
Contributor Author

@aoifeboyle that'd be great.

@kaushik94
Copy link
Contributor Author

@aoifeboyle You can just write the specs of rpacket and storage model on a doc somewhere and share it with me, I'll take care of the coding part. What do you think ?

@aoifeboyle
Copy link
Contributor

Hi @kaushik94, sure! I'll create a document on google docs.

@aoifeboyle
Copy link
Contributor

@kaushik94 I've updated our google doc with explanations of all of the parameters you asked for, and a bit of code to let you extract lines data from the atomic datasets for running your tests. Let me know if there is anything that isn't exactly clear! I hope this helps. If you need any other parameters explained, just put them in the google doc and I'll fill in explanations.

@aoifeboyle
Copy link
Contributor

@kaushik94 I added the new values you asked for to the google doc. Ping me next time you add new ones. It doesn't notify me automatically.

@aoifeboyle
Copy link
Contributor

@kaushik94 I got an email saying you'd left a comment here about your tests but I can't see it here - did you remove it? I looked at the tests in test_cmontecarlo.py anyway, and the only one that failed for me was test_rpacket_doppler_factor, but I was able to fix it by changing the doppler_factor variable for comparison slightly. Were you referring to something else? Or did you work it out?

@kaushik94
Copy link
Contributor Author

@aoifeboyle Yeah I mistakenly added a file that caused that error, I fixed it an deleted the comment. Are you getting an error still ? I just committed. The problem was that I din't declare js and nubars for the storage model. Seems fine now. I am trying to test montecarlo_one_packet_loop, what do you think ? Should I test any other functions before that ?

@aoifeboyle
Copy link
Contributor

Ah ok that's fine! And yeah I don't think it really matters which one you do next. Let me know if there are any more parameters you need me to define :)

@kaushik94
Copy link
Contributor Author

@aoifeboyle see the doc, just updated.

@aoifeboyle
Copy link
Contributor

Hi Kaushik,

How are you getting along now?

Aoife

Sent from Windows Mail

From: Kaushik Varanasimailto:[email protected]
Sent: ?Wednesday?, ?24? ?June? ?2015 ?12?:?50
To: tardis-sn/tardismailto:[email protected]
Cc: Aoife Boylemailto:[email protected]

@aoifeboylehttps://github.com/aoifeboyle see the doc, just updated.

Reply to this email directly or view it on GitHubhttps://github.com//pull/330#issuecomment-114842363.

@kaushik94
Copy link
Contributor Author

Hi @aoifeboyle I think I need your help, come on the doc if you are free now.

@aoifeboyle
Copy link
Contributor

I can't. I have to give my sister my laptop for the next hour or so. But I'll have a look at the doc then.

Sent from Windows Mail

From: Kaushik Varanasimailto:[email protected]
Sent: ?Monday?, ?29? ?June? ?2015 ?19?:?41
To: tardis-sn/tardismailto:[email protected]
Cc: Aoife Boylemailto:[email protected]

Hi @aoifeboylehttps://github.com/aoifeboyle I think I need your help, come on the doc if you are free now.

Reply to this email directly or view it on GitHubhttps://github.com//pull/330#issuecomment-116792711.

@aoifeboyle
Copy link
Contributor

@kaushik94 Done!

@kaushik94
Copy link
Contributor Author

@aoifeboyle strange because it seems to me that the problem is in thomson_scatter but I don't know why, it was running fine before. It most probably could be due to the montecarlo_one_packet function call but we can take a look at this later. For now I need some actual values.

/* The following values need to be
* reassigned properly. By default
* I assigned all 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.

@aoifeboyle I know this might take a while so sorry. I need some sensible values for the following. You can reply in the doc or even comment them in-line and please explain the terms if you have a little time. I thought of looking up in the atomic data but couldn't find much about the storage models and etc, or if you are busy just let me know where I can get the data elsewhere. Thanks a lot.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kaushik94 Having a look now. Let's try to keep the google doc more tidy (just for storing the parameter details) so it can be a reference for you. Let's keep the conversation on GitHub/email.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kaushik94 They're in the doc now. Most of it wasn't in the atomic data, but I've given you reasonable values (I think). Let me know if you have any more problems.

@aoifeboyle
Copy link
Contributor

@kaushik94 Did you see what I wrote about test in the doc? Your test works fine for me. So I don't know why it doesn't work for you? How do you actually run the tests?

@kaushik94
Copy link
Contributor Author

@aoifeboyle yes I saw, it was my mistake. The tests were running sometimes and sometimes fail, because I forgot to declare a function in C file which was giving weird errors. Now it all works fine. Thanks, will take a look at the doc.

@kaushik94
Copy link
Contributor Author

@aoifeboyle thanks, just see if I did everything correctly.

@kaushik94
Copy link
Contributor Author

@wkerzendorf review and merge.

@wkerzendorf
Copy link
Member

@kaushik94 it doesn't pass. can you elaborate what doesn't work.

@kaushik94
Copy link
Contributor Author

It works fine on my local. see this https://travis-ci.org/tardis-sn/tardis/jobs/69728700. strange error, says current_packet_id is not a member of storage model.

@kaushik94
Copy link
Contributor Author

I think there is a bug on the travis side or I am doing something terribly wrong. I am trying to debug but please let me know if you understood @wkerzendorf .

@wkerzendorf
Copy link
Member

@kaushik94 hmmm - try to clean your working directory with git clean -dfx and see if it still compiles with python setup.py test. as travis runs ubuntu that should be the same.

@kaushik94
Copy link
Contributor Author

Yes it does.

On Mon, Jul 6, 2015 at 8:04 PM, Wolfgang Kerzendorf <
[email protected]> wrote:

@kaushik94 https://github.com/kaushik94 hmmm - try to clean your
working directory with git clean -dfx and see if it still compiles with python
setup.py test. as travis runs ubuntu that should be the same.


Reply to this email directly or view it on GitHub
#330 (comment).

@kaushik94
Copy link
Contributor Author

@wkerzendorf I got the error. Check the storage.h file of the master in github. Someone removed current_packet_id parameter from the master.

@wkerzendorf
Copy link
Member

@kaushik94 yes that makes sense have a look at #338 that did that to parallelize the code. Can you rebase onto the current master?

@kaushik94
Copy link
Contributor Author

Okay got it.

@kaushik94
Copy link
Contributor Author

@wkerzendorf I am still getting an error
screen shot 2015-07-07 at 1 37 52 pm

@wkerzendorf
Copy link
Member

that's right - it's now called packet_id have a look at the code.

@kaushik94
Copy link
Contributor Author

@wkerzendorf please review and merge.

@kaushik94
Copy link
Contributor Author

Please don't merge, more functions to be added.

@kaushik94
Copy link
Contributor Author

@wkerzendorf please merge this. For reference #295 . I have not done testing for macro_atom because I needed more data.

@kaushik94
Copy link
Contributor Author

@wkerzendorf I am getting different answers for bf_cross_section on my local and travis. Travis expects 0 while on my local its -1. Can you take a look into the test_bf_cross_section function, I marked it with xfail.

@kaushik94
Copy link
Contributor Author

@wkerzendorf please merge this. But I don't know how to go about from here. A little help is much required. Thanks.

wkerzendorf added a commit that referenced this pull request Aug 3, 2015
[GSoC 2015 WIP] Testing other montecarlo functions
@wkerzendorf wkerzendorf merged commit 48dc3aa into tardis-sn:master Aug 3, 2015
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