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

Dev planning #22

Closed
henryiii opened this Issue Dec 19, 2016 · 6 comments

Comments

Projects
None yet
3 participants
@henryiii
Copy link
Member

henryiii commented Dec 19, 2016

These are my planned cleanups, in three steps:

  1. Cleanup make system (dev branch)
  • Remove globbing and the non-deterministic build failures caused by this
  • Remove fake nvcc and similar kludges
  • Use a central Makefile for examples
  • Simplify switch to OMP to only adding OMP_TARGET=1
  • Automate running (and timing) of all examples as a test system
  • Add separate run lines for each test in int_testing.py
  • Fix 4 remaining examples that currently (also in master?) fail
    • dalitz: std::logic_error, basic_string::_S_construct null not valid: Missing parameter (now has a default)
    • zachFit: Was missing a file (nice error message now)
    • pipipi0DPFit: Missing parameter (now gives nice error message)
    • chisquare: Missing parameter (now has a default)
  • let_testing.py skips tests that are missing files

Done! See PR #23 .

  1. Move files to more traditional structure (new folders branch)
  • New structure, including:
include/goofit
include/goofit/PDFs
include/goofit/rootstuff
src/goofit
src/PDFs
src/rootstuff
  • Including goofit files (in goofit and examples) would be:
#include "goofit/MyGoofitHeader.h"
  • File extension normalized (one ext for cpp, one for cuda, and one for headers)
  • Added script to automate this for easy transitions
  • Clean up formatting with astyle, include style file .astylerc

Done! Now in the dev branch.

  1. Move to CMake build system (better builds, make it easier to integrate Hydra ?
  • More compute capability choices, auto enables 3.5 extra features if possible, Auto option for GPU detection. Uses latest version of FindCUDA, supplied for CMake < 3.7
  • Optional separable compilation: cleaned up PDFs, added as option (on one system, multithreaded builds went from ~5 minutes to under 2 minutes)
  • Add auto Cuda/OMP option
  • Add Intel build
  • Add support for GooFit Packages
  • Allow packages/examples to add PDFs

Done! Now in the dev branch.

  1. Further questions:
  • Add documentation about building to readme
  • Remove unneeded warnings

Future work

  • Look at including Hydra
  • Look at setting up Travis CI (with low compute power tests!)
  • Add TBB - too hard for now, due to use of THREADIDX, BLOCKIDX
  • Improve separation of headers and source in PDFs
  • Look at removing the separate docs branch, and use docs in master (now supported in GitHub)
  • Make datafiles more readily available
  • Use some sort of parser in the examples to improve option descriptions
@hassec

This comment has been minimized.

Copy link
Member

hassec commented Dec 19, 2016

Hi @henryiii ,

looks like a great plan!
Just a quick note on the dalitz example, it seems like you missed that it needs to be run like ./dalitz dalitz_toyMC_000.txt. Without the text file it will throw the error you mentioned above.

@henryiii

This comment has been minimized.

Copy link
Member

henryiii commented Dec 20, 2016

Thanks @hassec, that was the problem. Added a default arg.

@galapaegos

This comment has been minimized.

Copy link
Contributor

galapaegos commented Dec 20, 2016

Hi @henryiii,

Dalitz and pipipi work fine. Chisquare works with a parameter. GPU/OMP toggling works out of the box minus the issues below.

ZachFit currently doesn't function due to the omp parallel calls causing multiple threads accessing the memory, PR#24 will fix this issue. ZachFit with OMP will not work due to specific cuda calls.

@henryiii

This comment has been minimized.

Copy link
Member

henryiii commented Dec 20, 2016

Thanks @galapaegos! I've fixed the (output) for the default parameter, and merged the PR. The only thing left is the odd zachFit behavior. It runs up to the print out of the coefficients, then just hangs and must be cancelled.

I believe all the specific cuda calls can be removed; setting device and checking compute compatibility should not be needed.

@galapaegos

This comment has been minimized.

Copy link
Contributor

galapaegos commented Dec 20, 2016

Hi @henryiii,

ZachFit is hanging on a missing file. It is expecting 'dataFiles/zach/dstwidth_kpi_data.dat', but is only provided the 'dataFiles/dstwidth_kpi_resMC.dat'.

@henryiii

This comment has been minimized.

Copy link
Member

henryiii commented Dec 20, 2016

Yes, I realized that and have added an error message for it. Trying to find a copy of that file to test. I assume it's one we can't make available. We might be able to add the files in dropbox that pipipi0 need to GitHub downloads.
(I might be thinking of bitbucket, actually)

@henryiii henryiii closed this Jan 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