villaa
commented
2 years ago @matthewfeickert I cannot reproduce this--please include your system specs.
Closed villaa closed 2 years ago
villaa
commented
2 years ago @matthewfeickert I cannot reproduce this--please include your system specs.
matthewfeickert
commented
2 years ago It is the Docker image included in the summary. https://github.com/openjournals/joss-reviews/issues/3932#issuecomment-990735982
Though it is nothing specific to that Docker image, as I can reproduce the same thing locally.
villaa
commented
2 years ago Ok, that's docker image made by:
docker run --rm -ti -v $PWD:$PWD -w $PWD atlasamglab/stats-base:root6.24.06
matthewfeickert
commented
2 years ago If you run though the example I posted in full it seems that the MersenneTwister code that you vendor is not C++17 compliant. If you aren't going to support anything above c++14 that need to be made clear in the project, but that also means that the software is severely limited.
villaa
commented
2 years ago If you run though the example I posted in full it seems that the
MersenneTwistercode that you vendor is not C++17 compliant. If you aren't going to support anything abovec++14that need to be made clear in the project, but that also means that the software is severely limited.
yes, we understand this issue. There is a separate issue in github about it (#27). And it is also the reason for the "hard-coding" in the Makefile. The team doesn't currently care about compatibility with a wide range of compilers, so I'm trying not to spend time on that; and the API for the new MT seems to have changed. But it may be inevitable that all these issues depend on #27
matthewfeickert
commented
2 years ago The team doesn't currently care about compatibility with a wide range of compilers, so I'm trying not to spend time on that
Hopefully the other issues will resolve this en route, as the current pinning to c++14 limits the general use of the software.
Now that I think more about it though, this behavior is just weird and I think shows a bug. Anything written in c++14 should compile in c++17 given that backwards compatibility is a C++ strength, so the fact that things aren't working I think is a sign of buggy behavior (though you may already have this noted in another Issue).
the API for the new MT seems to have changed. But it may be inevitable that all these issues depend on #27
Okay, sounds good as it seems that you have a clear focus on the work that needs to be done there! :+1:
If it helps at all for testing, I've answered part of my question in https://github.com/villano-lab/nrCascadeSim/issues/45#issuecomment-993109070 as it seems that the ROOT team still compiles their Docker releases with the oldest supported version for the time being, even if LCG doesn't.
However, this gives the same failing behavior as I see on c++17
$ docker run --rm -ti rootproject/root:6.24.06-ubuntu20.04 /bin/bash
# apt update -y && apt install -y -qq git
# cd /tmp/
# git clone --depth 1 https://github.com/villano-lab/nrCascadeSim.git --branch v1.2.3 --single-branch
# cd nrCascadeSim/
# root --version
ROOT Version: 6.24/06
Built for linuxx8664gcc on Sep 02 2021, 14:20:23
From tags/v6-24-06@v6-24-06
# root-config --cflags
-pthread -std=c++14 -m64 -I/opt/root/include
# make
g++ -fPIC -c -D__GIT_VERSION=\"v1.2.3\" -IMersenne -Iinc src//isotope_info.c `root-config --cflags --glibs` -std=c++14 -L. -L/tmp/nrCascadeSim/bin/lib
mv isotope_info.o /tmp/nrCascadeSim/bin/lib/
g++ -fPIC -c -D__GIT_VERSION=\"v1.2.3\" -IMersenne -Iinc src//weisskopf.c `root-config --cflags --glibs` -std=c++14 -L. -L/tmp/nrCascadeSim/bin/lib
mv weisskopf.o /tmp/nrCascadeSim/bin/lib/
g++ -fPIC -c -D__GIT_VERSION=\"v1.2.3\" -IMersenne -Iinc src//lindhard.c `root-config --cflags --glibs` -std=c++14 -L. -L/tmp/nrCascadeSim/bin/lib
mv lindhard.o /tmp/nrCascadeSim/bin/lib/
g++ -fPIC -c -D__GIT_VERSION=\"v1.2.3\" -IMersenne -Iinc src//cascadeProd.c `root-config --cflags --glibs` -std=c++14 -L. -L/tmp/nrCascadeSim/bin/lib
src//cascadeProd.c: In function ‘cri* Cascade(int, int, double, int, double*, double*, double, MTRand*)’:
src//cascadeProd.c:1113:1: warning: control reaches end of non-void function [-Wreturn-type]
1113 | }
| ^
mv cascadeProd.o /tmp/nrCascadeSim/bin/lib/
g++ -fPIC -c -D__GIT_VERSION=\"v1.2.3\" -IMersenne -Iinc src//edepmath.c `root-config --cflags --glibs` -std=c++14 -L. -L/tmp/nrCascadeSim/bin/lib
mv edepmath.o /tmp/nrCascadeSim/bin/lib/
g++ -fPIC -c -D__GIT_VERSION=\"v1.2.3\" -IMersenne -Iinc src//rootUtil.c `root-config --cflags --glibs` -std=c++14 -L. -L/tmp/nrCascadeSim/bin/lib
mv rootUtil.o /tmp/nrCascadeSim/bin/lib/
g++ -fPIC -shared /tmp/nrCascadeSim/bin/lib/lindhard.o /tmp/nrCascadeSim/bin/lib/weisskopf.o /tmp/nrCascadeSim/bin/lib/isotope_info.o /tmp/nrCascadeSim/bin/lib/cascadeProd.o /tmp/nrCascadeSim/bin/lib/edepmath.o /tmp/nrCascadeSim/bin/lib/rootUtil.o `root-config --cflags --glibs` -std=c++14 -o /tmp/nrCascadeSim/bin/lib/libncap.so
g++ -fPIC -Wl,-rpath /tmp/nrCascadeSim/bin/lib -D__GIT_VERSION=\"v1.2.3\" -IMersenne -Iinc -L. -L/tmp/nrCascadeSim/bin/lib /tmp/nrCascadeSim/bin/realizeCascades.cpp `root-config --cflags --glibs` -std=c++14 -lncap -o /tmp/nrCascadeSim/bin/realizeCascades
# make install
cp /tmp/nrCascadeSim/bin/realizeCascades /usr/local/bin/
# realizeCascades --verbose -n 100000 -o output.root levelfiles/Si28_ngam_all_cascades_rfmt_sorted.txt
levelfiles/Si28_ngam_all_cascades_rfmt_sorted.txt
Seed used: 690075872
MTRand: 0x55ebae093520
root@bf99907b8b24:/tmp/nrCascadeSim# root -l output.root
root [0]
Attaching file output.root as _file0...
(TFile *) 0x55a43e6720d0
root [1] _file0->ls()
TFile** output.root
TFile* output.root
KEY: TTree cascade;1 cascade
root [2] cascade->Print()
******************************************************************************
*Tree :cascade : cascade *
*Entries : 0 : Total = 329 bytes File Size = 200 *
* : : Tree compression factor = 1.00 *
******************************************************************************
root [3]This also backs up my thoughts from above that there is some buggy behavior in the examples.
villaa
commented
2 years ago I think our solution to issue #12 introduced a bug where the cascades only get written when the verbosity flag is on. This wasn't showing up in the testing because we simply got an md5 sum of the output file, but never check that the output file has data.
matthewfeickert
commented
2 years ago I think our solution to issue #12 introduced a bug where the cascades only get written when the verbosity flag is on.
One final piece of maybe helpful info for debugging: The lack of file content is independent of if --verbose is given or not
# ...continuing in the same Docker image as above in this Issue
root@6cd75515d66f:/tmp/nrCascadeSim# realizeCascades -n 100000 -o output.root levelfiles/Si28_ngam_all_cascades_rfmt_sorted.txt
Seed used: 2054424945
MTRand: 0x5566c46e6520
root@6cd75515d66f:/tmp/nrCascadeSim# realizeCascades --verbose -n 100000 -o output_verbose.root levelfiles/Si28_ngam_all_cascades_rfmt_sorted.txt
levelfiles/Si28_ngam_all_cascades_rfmt_sorted.txt
Seed used: 11983732
MTRand: 0x55ebbbe8b520
root@6cd75515d66f:/tmp/nrCascadeSim# root -l output.root
root [0]
Attaching file output.root as _file0...
(TFile *) 0x55f6ed1537d0
root [1] _file0->ls()
TFile** output.root
TFile* output.root
KEY: TTree cascade;1 cascade
root [2] cascade->Print()
******************************************************************************
*Tree :cascade : cascade *
*Entries : 0 : Total = 329 bytes File Size = 200 *
* : : Tree compression factor = 1.00 *
******************************************************************************
root [3] .q
root@6cd75515d66f:/tmp/nrCascadeSim# root -l output_verbose.root
root [0]
Attaching file output_verbose.root as _file0...
(TFile *) 0x55f469baac80
root [1] _file0->ls()
TFile** output_verbose.root
TFile* output_verbose.root
KEY: TTree cascade;1 cascade
root [2] cascade->Print()
******************************************************************************
*Tree :cascade : cascade *
*Entries : 0 : Total = 329 bytes File Size = 200 *
* : : Tree compression factor = 1.00 *
******************************************************************************
root [3] but I'll stop commenting here and I'll just follow up in the review as you already have things up and tracking again in Issue #12, which is great. :+1:
villaa
commented
2 years ago I think our solution to issue #12 introduced a bug where the cascades only get written when the verbosity flag is on.
One final piece of maybe helpful info for debugging: The lack of file content is independent of if
--verboseis given or not
Verbosity has many levels. The bug makes it so results are only printed at level 2 or above. try --verbose=2 or -v2. In any case we are trying to fix the bug in #12 which was re-opened.
matthewfeickert
commented
2 years ago Verbosity has many levels. The bug makes it so results are only printed at level 2 or above. try --verbose=2 or -v2.
Ah okay. :+1: I'd suggest also improving the help for the CLI then as
realizeCascades --help
Usage: (null) options [ inputfile(s) ]
-d, --seed <integer> seed for random numbers
-h, --help print usage
-n, --numgen <number> number of traces to generate
-o, --outfile <filename> name the output file
-s, --silent silent, no standard out
-v, --verbose <level> Print verbose messages at level <level>
-V, --version print version and exit
-l, --log <filename> Log additional output to the specified file. If this option is not used, no logging will occur.makes it seems like you would use
--verbose 2which will just run happily doing nothing without giving any errors or warnings. Requiring the = for
--verbose=2isn't clear at the moment.
villaa
commented
2 years ago We use the GNU getopt, I don't know why the equal sign is required in this post it is claimed that you should be able to do either. However, now that I think about it the final argument to this program has no flag and the argument to --verbose is optional so I don't know how getopt would know the difference between --verbose 2 input_file and --verbose 2 input_file where in the latter case 2 is the name of the first input file (input_file is the second input file) and verbose is given without a "level."
Come to think of it, have we tested the multiple input-file appropriately?
Incidentally there are also some standard behaviors of the --help and --version options that we don't currently respect, so those should probably be upgraded. Note these are GNU software standards.
villaa
commented
2 years ago Note: here is a list of software licenses for the --version output.
nuclearGoblin
commented
2 years ago Currently having the same issue even when using --verbose=2 or --verbose=3.
villaa
commented
2 years ago Currently having the same issue even when using
--verbose=2or--verbose=3.
@gerudo7 which version are you using? I thought I fixed this in v1.2.4 (hotfix)
nuclearGoblin
commented
2 years ago @villaa sorry, was just coming back to update - I thought I'd checked everything before commenting, but it looks like I was using an obsolete levelfile (to be clear - one that was removed from the repository but was left over locally because of some old work). It is working for me now
villaa
commented
2 years ago This was essentially fixed b/c PR #54 allowed PR #57 and now the C++17 on Ubuntu20 is being explicitly tested in Travis and is passing on v1.2.4.
The first example that a user encounters in the Executables of nrCascadeSim section of the docs fails. It will run without producing warning or errors, but if run from the top level of the project repo so that levelfiles/Si28_ngam_all_cascades_rfmt_sorted.txt is in the given example path the content of the resulting ROOT TTree in the file is empty
This doesn't seem correct given that the example says it will:
As the project includes over 30 levelfiles it would be nice to show specific run examples and output in the docs as well rather than just example source.