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

GooFit IO & Variable #68

Merged
merged 78 commits into from May 23, 2017

Conversation

Projects
None yet
2 participants
@henryiii
Copy link
Member

henryiii commented May 3, 2017

The GooFit IO system has been revamped under the hood, providing cleaner, faster data input (address portions of #34). This PR does not change the conversion to CUDA that occurs in the PDF.

  • Drop maps and other oddities in DataSets
  • Remove useless typedefs
  • More tests, including some that fixed bugs with Minuit1, default to building tests now
  • Lots more C++11 cleanup
  • RunAll.py is beginning to be fast enough to run as test, with slower --profile option
  • Logging and Error reporting improvements
  • zachFit now runs if dataset in ./dataFiles
  • Default to separable compilation (#69)
  • Tightened the API of Variable.
    • Allow BinnedDataSet to lock the number of bins, use Variable directly
  • Better readme with more help for conversion from older code
  • Drop assert in favor of GooFit::GeneralError
  • Remove GenVoigtian (unused, duplicate) and more rootstuff files
  • DataSet iteratation was available partway through the patch, which iterated variables. Might not be ideal (naive user might think iteration means over events), so getVariables() was optimized and iteration was removed.
  • addEvent(value) is now variadic instead of being a special case.
  • addEvent now throws error if out of range; variable can be converted to bool (true if in range)
  • Python bindings now build with Travis, one test added (#63)
  • CMake now checks for Backtrace (needed on some Linux systems, like Alpine docker)
  • Fixed one more case of .cu inclusion!
  • Evaluate at points now returns the result
  • Added plotToROOT if ROOT is present; 1D (for now) plot to histogram of PDF.
  • Removed CMake in-source builds, restored make shortcut (make in main dir calls CMake in build dir)
  • Rethought and restored blinding variables
  • Directory linking now happens at configure time, much cleaner IDE target list, should configure with Visual Studio
  • FCN can be directly calculated
@codecov

This comment has been minimized.

Copy link

codecov bot commented May 4, 2017

Codecov Report

Merging #68 into master will increase coverage by 18.86%.
The diff coverage is 62.14%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #68       +/-   ##
===========================================
+ Coverage   39.85%   58.71%   +18.86%     
===========================================
  Files          25       39       +14     
  Lines        1119     1233      +114     
  Branches      152      165       +13     
===========================================
+ Hits          446      724      +278     
+ Misses        673      509      -164
Impacted Files Coverage Δ
include/goofit/PdfBase.h 45.45% <ø> (-22.97%) ⬇️
include/goofit/fitting/FCN.h 100% <ø> (ø) ⬆️
include/goofit/Error.h 33.33% <0%> (+33.33%) ⬆️
src/PDFs/ExpPdf.cu 36.06% <0%> (+1.06%) ⬆️
python/DataSet.cpp 63.63% <0%> (ø)
include/goofit/PDFs/GooPdf.h 33.33% <100%> (+13.33%) ⬆️
python/BinnedDataSet.cpp 100% <100%> (ø)
include/goofit/UnbinnedDataSet.h 100% <100%> (ø) ⬆️
include/goofit/DataSet.h 100% <100%> (+42.85%) ⬆️
python/PdfBase.cpp 100% <100%> (ø)
... and 37 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 323dc20...d264868. Read the comment docs.

henryiii added some commits May 4, 2017

@henryiii henryiii changed the title [WIP] Fitfrac [WIP] GooFit IO May 4, 2017

@henryiii henryiii changed the base branch from logging to master May 4, 2017

@henryiii henryiii changed the title [WIP] GooFit IO & Variable GooFit IO & Variable May 23, 2017

@henryiii

This comment has been minimized.

Copy link
Member

henryiii commented May 23, 2017

I think that fixed the broken example(s). Using set instead of vector was changing order, and some PDFs cared.

@henryiii

This comment has been minimized.

Copy link
Member

henryiii commented May 23, 2017

@galapaegos does this work now? At least as well as master?

@galapaegos

This comment has been minimized.

Copy link
Contributor

galapaegos commented May 23, 2017

I performed a fresh checkout of this branch. Only one example (exponential) is getting compiled. My CMake arguments are: cmake3 ../ -DGOOFIT_SEPARATE_COMP=ON -DGOOFIT_ARCH=6.0, and compiled with make -j 12.

I manually ran the tests also. The UnbinnedFit fails, here is an excerpt:

The minimization took: 35.613 ms
Average time per call: 429.32 us
[       OK ] UnbinnedFit.DualFit (41 ms)
[ RUN      ] UnbinnedFit.DifferentFitterVariable
MnSeedGenerator: for initial parameters FCN = 6208951.980774
MnSeedGenerator: Initial state:   - FCN =   6208951.980774 Edm =   3.6176e+06 NCalls =     25
VariableMetric: start iterating until Edm is < 0.0001
VariableMetric: Initial state   - FCN =   6208951.980774 Edm =   3.6176e+06 NCalls =     25
VariableMetric: Iteration #   0 - FCN =   6208951.980774 Edm =   3.6176e+06 NCalls =     25
VariableMetric: Iteration #   1 - FCN =   2443093.115322 Edm =       532136 NCalls =     50
VariableMetric: Iteration #   2 - FCN =   632894.3747686 Edm =       585147 NCalls =     80
VariableMetric: Iteration #   3 - FCN =   350936.7830007 Edm =      78543.3 NCalls =     96
VariableMetric: Iteration #   4 - FCN =   349824.9423883 Edm =      22236.6 NCalls =    119
VariableMetric: Iteration #   5 - FCN =   349802.9215145 Edm =      386.597 NCalls =    139
Info: DavidonErrorUpdator: delgam < 0 : first derivatives increasing along search line
VariableMetric: Iteration #   6 - FCN =   349468.2558349 Edm =  1.40282e+06 NCalls =    159
Info: VariableMetricBuilder: matrix not pos.def, gdel > 0
Info: gdel = 635227
Info in negative or zero diagonal element in covariance matrix : i = 2
Info in added to diagonal of Error matrix a value : dg = 0.500001
Info: gdel = -2.17635e+14
Abort called from /home/bhittle/goofit/src/PDFs/GooPdf.cu line 294 due to totalpdf zero NLL
Parameters of totalpdf :
  xalpha (2) :  -0.166274
  xsigma (3) :  0.00413787
  yalpha (0) :  6.86755
  ysigma (1) :  0.4135
Parameters (38) :
6.86755 0.4135 -0.166274 0.00413787 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
./UnbinnedTest[0x53a2c6]
./UnbinnedTest[0x48a9a1]
./UnbinnedTest[0x542d6c]
./UnbinnedTest[0x58c114]
./UnbinnedTest[0x588713]
./UnbinnedTest[0x5731b1]
./UnbinnedTest[0x576cfe]
./UnbinnedTest[0x56d868]
./UnbinnedTest[0x56c5e9]
./UnbinnedTest[0x5586e5]
./UnbinnedTest[0x53c939]
./UnbinnedTest[0x40eba1]
./UnbinnedTest[0x487b73]
./UnbinnedTest[0x47ab47]
./UnbinnedTest[0x47abee]
./UnbinnedTest[0x47acf5]
./UnbinnedTest[0x47afa8]
./UnbinnedTest[0x47b254]
./UnbinnedTest[0x408432]
/usr/lib64/libc.so.6(__libc_start_main+0xf5)[0x7ff72732fb35]
unknown file: Failure
C++ exception with description "GeneralError: totalpdf zero NLL" thrown in the test body.
[  FAILED  ] UnbinnedFit.DifferentFitterVariable (44 ms)
[----------] 3 tests from UnbinnedFit (742 ms total)

[----------] Global test environment tear-down
[==========] 3 tests from 1 test case ran. (743 ms total)
[  PASSED  ] 2 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] UnbinnedFit.DifferentFitterVariable

 1 FAILED TEST

Please advise, thanks!

@henryiii

This comment has been minimized.

Copy link
Member

henryiii commented May 23, 2017

A few comments:

  • If only exponential is getting compiled, that means that you don't have ROOT. On Goofy, that's ml root. On any other computer, source thisroot.sh. Also, make -j24 is faster on Goofy
  • You don't need to specify GOOFIT_SEPARATE_COMP, that defaults to ON
  • You can build for multiple arch's now, auto-detection should work

I'm looking into the test error now, that shouldn't be happening... It might be related to the stand-alone Minuit2, which I don't test as thoroughly yet.

@henryiii

This comment has been minimized.

Copy link
Member

henryiii commented May 23, 2017

Useful .bashrc or .bash_profile settings:

export MAKEFLAGS="-j24"
export CTEST_OUTPUT_ON_FAILURE=1
export GTEST_COLOR=1
@galapaegos

This comment has been minimized.

Copy link
Contributor

galapaegos commented May 23, 2017

Great, those are helpful thanks!

The examples compile now that root is in the path.

@henryiii

This comment has been minimized.

Copy link
Member

henryiii commented May 23, 2017

The standalone Minuit doesn't quite work properly. It fails on the final unbinned test, and occasionally on one of the binned tests. This is not better/worse than master, though.

henryiii added some commits May 23, 2017

@henryiii

This comment has been minimized.

Copy link
Member

henryiii commented May 23, 2017

The Minuit2 standalone version should work correctly now. OpenMP ifdefs were activated, but the OpenMP implementation in Minuit2 is buggy and unfinished.

@galapaegos

This comment has been minimized.

Copy link
Contributor

galapaegos commented May 23, 2017

Everything runs fine for me.

@henryiii

This comment has been minimized.

Copy link
Member

henryiii commented May 23, 2017

Add a review and I'll merge, then.

@galapaegos
Copy link
Contributor

galapaegos left a comment

Approving additions to support fitting fractions.

@henryiii henryiii merged commit 101631b into master May 23, 2017

4 checks passed

codecov/patch 62.14% of diff hit (target 39.85%)
Details
codecov/project 58.71% (+18.86%) compared to 323dc20
Details
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 fitfrac branch May 23, 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