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

Fn/eventual fixing lo sand sub #88

Open
wants to merge 67 commits into
base: master
Choose a base branch
from

Conversation

FabienNugier
Copy link
Contributor

The main contribution in this branch concerns the substructure. I have rewritten the organization of the functions insert/reset/deleteSubstructure(s) () and tested them in many different configurations (including also low numberdensity that generates no halos). I merged also with the master branch and solved conflicts. I should review all the changes but I think it will not create problems.

… the moment implemented only for substructure. It does not work so well and it needs to be developed further.
…er field planes than the substructure plane.
…s some of the properties of the substructure. Also several changes were made in Lens::insertSubstructures, Lens::resetSubstructure, and Lens::deleteSubstructures in order to avoid some errors happening when there is no substructure halos.
…hat I can use them in my driver program. Also noticed that before rho of substructure was not actually saved.
FabienNugier and others added 18 commits October 4, 2016 16:46
… the substructure halos in insertSubstructures and resetSubstructure. Corrected a typo with the lengths[] test in TreeQuad::BuildQTreeNB.
…the parameter file halos and not getting the error from assert(Dist == -1).
…d Lens::replaceMain().

Also imposed some more discipline  on the LenHalo hierarchy by making zlens, mass and Rsize private
variables of LensHalo and do the assignment of these variables from the parameter file through the
LensHalo constructor only.
@FabienNugier
Copy link
Contributor Author

I think we should do the merge pull request now as everything seems to work fine on SLsimLib side.

@rbmetcalf
Copy link
Member

The problem is that when I diff the head of your branch with master there are a
large number of changes that I don't understand or are redundant. Merging
it to master would risk the stability of master.

I think the best way to proceed is for you to find the commits on your branch and
cherry-pick them onto new branches, one for each self contained change or fix.
We make a pull request out of each that can be reviewed and isolated and we
can sort out some of the extra stuff.

@FabienNugier
Copy link
Contributor Author

Okay. So if I understand well I put myself on the master branch, then I create a new branch and cherry pick the commits one by one according to their "meaning", but not particularly according to their date, right ? What should I do with the commits which consisted in merging the master branch into my own branch (and eventually fixing conflicts) ?

@FabienNugier
Copy link
Contributor Author

I am also wondering: which is the first commit I should do the cherry-pick on ? Dec 1, 2015, right ? The commits before where already merged I think.

@rbmetcalf
Copy link
Member

Hi Fabien,

You should do the cherry picking in chronological order. Otherwise you might
contradict with yourself. You should start from when the branch was created or
the last time it was merged into the master. You should ignore mergers into your
branch because they will exist in the master already.

The priority is on getting the paper done though.

@FabienNugier
Copy link
Contributor Author

Hi Ben,
I have done the cherry picking. For that I created a new branch from the head of the master branch, and I was cherry picking from my old branch to it. I had to resolve some conflicts, sometimes things looking a bit strange but not really bit changes. Nevertheless, my code is not working any more with these cherry picks... the simple construction of a lens for example does not work. I will try to investigate but I don't understand why this happens since I was often pull from the master branch into my branch. I did not cherry-picked the commits coming from merged branches (it is actually impossible to do), and maybe some commits are missing, but I guess if that was true I would have got much more conflicts to solve... so I don't know, I have to see point by point how to correct the problems... I'll try to do that this week if I have time.
Best regards,
Fabien

@FabienNugier
Copy link
Contributor Author

I did not start the cherry pick from the moment the branch (fn/EventualFixingLoSandSub) was created though, but instead from the last time it was merged into the master branch. I guess this is not a problem.

@rbmetcalf
Copy link
Member

Lets concentrate on the research and worry about this later.

@FabienNugier
Copy link
Contributor Author

I solved the problems I think and it seems to work with my project now. I am running it over the different lenses in order to check with previous runs I had done and see if anything is affected. Maybe we can merge the cherry-picked branch right after I have proceeded with a visual check of my results (compared with previous runs) ? Hence this problem will be solved and we can focus on the physics indeed :).

@FabienNugier
Copy link
Contributor Author

Okay I checked that the results are exactly the same between now (on my cherry pick branch) and previously on my local branch. I had merged the master branch into my cherry pick branch this morning and there was no conflict. Should I do a pull request of my cherry-pick branch ?

@rbmetcalf
Copy link
Member

Yes, that would be more easy to review.

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.

2 participants