search

gandalfcode / gandalf

GANDALF (Graphical Astrophysics code for N-body Dynamics And Lagrangian Fluids)
GNU General Public License v2.0
44 stars 12 forks source link

Papertests #71

Closed dhubber closed 7 years ago

dhubber commented 7 years ago

Restructuring of tests directory including adding papertests sub-directory containing all parameters files and python scripts for generating figures for gandalf paper. Also various small code bug fixes.

rbooth200 commented 7 years ago

Looking at this I don't think we can accept the pull request since the branch has become quite a mess. My complaints are: 1) It is built on top of the tests branch, which we haven't merged because it needs fixing. 2) Many commits are included multiple times 3) It doesn't pass travis. 4) The branch diverged from master a long time ago

I would suggest that we cherry-pick the commits we need. I think those that are related to travis should probably added to the test branch and dealt with there. The rest should perhaps be added to a new branch that is based on the current master from which we should make the pull request.

dhubber commented 7 years ago

Yes, looking now at the commit history, I agree it looks messy, especially with point 2 which I find confusing (I thought rebasing was supposed to lead to a cleaner commit history and not a total mess, but I guess not). So I'm not entirely sure how that happened but anyway, we should try to clean it up I agree. But not sure about your other points; 1 and 3 are related to the tests branch (original intention was to merge in tests once fixed but maybe not now) and 4 doesn't really matter if we find a solution to 2. But I will have a look at the cherry picking option now to see how best to do this to clean up the mess. If that leads to a new branch, then I'll close this and submit a new pull request.

rbooth200 commented 7 years ago

I was going to have a go at doing precisely this. I'm happy to leave to you if you'd like though.

rbooth200 commented 7 years ago

However, I think that we should definitely not be merging things to master that do not pass the travis checks, even if the problem is with the checks themselves.

dhubber commented 7 years ago

ok, I've prepared a new branch by cherry-picking and have pushed it to github also (papertests2). Just waiting on the Travis tests now.

dhubber commented 7 years ago

ok, seems to have worked (i.e. passed the Travis tests) so I'll close this pull request and create a new one with the newer branch