Skip to content

GooFit IO & Variable #68

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 78 commits into from
May 23, 2017
Merged

GooFit IO & Variable #68

merged 78 commits into from
May 23, 2017

Conversation

henryiii
Copy link
Member

@henryiii henryiii commented May 3, 2017 •
edited
Loading

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 (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 (Python bindings added to CI build #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
Copy link

codecov bot commented May 4, 2017 •
edited
Loading

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 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 16:20
@henryiii henryiii changed the title [WIP] GooFit IO & Variable GooFit IO & Variable May 23, 2017
@henryiii
Copy link
Member Author

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

@henryiii
Copy link
Member Author

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

@galapaegos
Copy link
Contributor

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
Copy link
Member Author

henryiii commented May 23, 2017 •
edited
Loading

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
Copy link
Member Author

Useful .bashrc or .bash_profile settings:

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

@galapaegos
Copy link
Contributor

Great, those are helpful thanks!

The examples compile now that root is in the path.

@henryiii
Copy link
Member Author

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
Copy link
Member Author

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

@galapaegos
Copy link
Contributor

Everything runs fine for me.

@henryiii
Copy link
Member Author

Add a review and I'll merge, then.

galapaegos
galapaegos approved these changes May 23, 2017
View reviewed changes
Copy link
Contributor

@galapaegos galapaegos left a comment

Choose a reason for hiding this comment

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

Approving additions to support fitting fractions.

@henryiii henryiii merged commit 101631b into master May 23, 2017
@henryiii henryiii deleted the fitfrac branch May 23, 2017 16:20
@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
Reviewers

@galapaegos galapaegos galapaegos approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
V 2.0
Development

Successfully merging this pull request may close these issues.
2 participants
@henryiii @galapaegos