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

No rootstuff #56

Merged
merged 37 commits into from Apr 21, 2017

Conversation

Projects
None yet
2 participants
@henryiii
Copy link
Member

henryiii commented Apr 4, 2017

This removes the old rootstuff directory, since GooFit for the moment requires ROOT. This eventually should have minuit or minuit2 added from a separate source so that GooFit will not require ROOT again.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Apr 4, 2017

Codecov Report

Merging #56 into master will increase coverage by 13.61%.
The diff coverage is 78.75%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #56       +/-   ##
===========================================
+ Coverage   33.72%   47.33%   +13.61%     
===========================================
  Files          25       24        -1     
  Lines        6571     1126     -5445     
  Branches     1261      145     -1116     
===========================================
- Hits         2216      533     -1683     
+ Misses       4355      593     -3762
Impacted Files Coverage Δ
include/goofit/UnbinnedDataSet.h 100% <ø> (ø) ⬆️
include/goofit/Variable.h 75% <ø> (ø) ⬆️
include/goofit/BinnedDataSet.h 0% <ø> (ø) ⬆️
include/goofit/fitting/FCN.h 100% <100%> (ø)
src/goofit/FitControl.cc 52.38% <100%> (ø) ⬆️
include/goofit/fitting/Params.h 100% <100%> (ø)
include/goofit/fitting/FitManagerMinuit2.h 100% <100%> (ø)
src/goofit/FitManagerMinuit2.cc 100% <100%> (ø)
src/goofit/PdfBase.cc 29.91% <20%> (ø) ⬆️
src/goofit/Variable.cc 28.2% <33.33%> (-5.13%) ⬇️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb132af...dd3dbd2. Read the comment docs.

@henryiii

This comment has been minimized.

Copy link
Member

henryiii commented Apr 18, 2017

I won't be able to work on this for a day or two, but currently there is an issue partially related to this branch. The DP4 example fails with the new Minuit2 fitter for any backend with a NLL=0 error, or with the old Minuit1 fitter only with debugging enabled (which is making it hard to track down). This makes me suspect uninitialized memory access, perhaps in AddPdf. @galapaegos, this might be related to the MPI changes, I haven't tried running with pre-MPI code yet but plan to.

@galapaegos

This comment has been minimized.

Copy link
Contributor

galapaegos commented Apr 19, 2017

Just a comment on this, Minuit1 with Release+debug will solve but Minuit2 does not for DP4. Would the expectation be that Minuit1 and Minuit2 come to the same or very close result? I'm getting different results beyond 0.01 between Minuit1 and Minuit2, and the error is a larger error difference, with Minuit1 having the larger error in general.

Definitely possible there is an MPI bug; the non-MPI path should be exactly the same as the original excluding the RO_CACHE changes. The RO_CACHE changes will trigger a crash not a parameter assertion or sum assertion if used as a fakeEvent.

@henryiii

This comment has been minimized.

Copy link
Member

henryiii commented Apr 19, 2017

I believe the Minuit2 has better defaults; it seems to perform a HESSE like calculation automatically. If explicitly called, I believe you can perform the exact same procedures, but I tried to stay close to the default for FitManager. So it might be slightly better, but should not be very different.

(Will try to look into this further to ensure the results are correct)

henryiii added some commits Apr 19, 2017

@henryiii henryiii changed the title [WIP] No rootstuff No rootstuff Apr 20, 2017

@henryiii henryiii merged commit c234008 into master Apr 21, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@henryiii henryiii deleted the no_rootstuff branch Apr 21, 2017

@henryiii henryiii modified the milestone: V 2.0 Jun 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment