Skip to content
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

Consolidate file locations #6

Closed
tunnell opened this issue Jul 8, 2018 · 11 comments
Closed

Consolidate file locations #6

tunnell opened this issue Jul 8, 2018 · 11 comments
Assignees

Comments

@tunnell
Copy link
Member

tunnell commented Jul 8, 2018

When trying to make programs that use the nest library, an issue I encounter is the sporadic location of where header files are located. Can we do the following:

  • G4 bindings move into a new repository
  • Examples in their own folder
  • NEST and VDetector header in same location
  • NEST and VDetector source files in same location
@mszydagis
Copy link
Collaborator

  • Not moving G4 bindings into a new repo. Sub-folder good enough. I don't want to manage a separate repo that needs the NEST repo called separately.
  • Examples are already in their own folder (Tools). I don't want more sub-folders, I like the way it is now.
  • Talk to Jacob. This will annoy people on LUX and LZ again as yet another file location change. There is reason for it though with Detectors as a separate subfolder. This would require not only moving files but also re-writing README instructions on how to implement a new detector.
  • Talk to Jacob. This will annoy people on LUX and LZ again as yet another file location change.

@tunnell
Copy link
Member Author

tunnell commented Jul 8, 2018

We can ask Jacob. I just meant testSpectra and Testnest

@jpbrodsky
Copy link
Collaborator

jpbrodsky commented Jul 9, 2018 via email

@mszydagis
Copy link
Collaborator

Actually, Jason, I fear you are behind the times by several weeks. The headers have been moved around by Jacob and/or myself recently.

@jpbrodsky
Copy link
Collaborator

jpbrodsky commented Jul 9, 2018 via email

@tunnell
Copy link
Member Author

tunnell commented Jul 9, 2018

@jpbrodsky I meant the file locations within the repository. In this case, all the source and header files are spread out a bit. You're right that if somebody is just linking to the library, then after install, you have the headers in one place. I guess I should have been more clear: some ways that you can bind other languages to C++ require some sort of reflection so you know where all of the headers and cpp files are such that they can be 'patched' with the binding description (https://github.com/NESTCollaboration/nestpy). At least the way people recommended it to me, you ship the raw code with the bindings and let it compile during pip install if no prebuilt binary exists for your setup.

The whole issue is not that big a deal (can skip this release or completely). My impression just of the repository itself was that the code locations felt nonstandard to me, but it's been a while since I've done any heavy C++ programming.

On the install location, do you want to make a quick pull request with the change about the install instructions?

@jpbrodsky
Copy link
Collaborator

jpbrodsky commented Jul 9, 2018 via email

@tunnell
Copy link
Member Author

tunnell commented Jul 9, 2018

Closed by #11

@tunnell tunnell closed this as completed Jul 9, 2018
@tunnell tunnell reopened this Jul 9, 2018
@tunnell
Copy link
Member Author

tunnell commented Sep 18, 2019

The changes would be:

  • Rename Tools to examples

  • Create src

  • testNEST.cpp -> Tools/

  • testSpectra.cpp -> Tools/

  • Detectors/DetectorExample_XENON10.hh -> include

  • Detectors/VDetector.hh.hh -> include

  • NEST.cpp -> src

  • G4integration/ -> examples

  • Delete empty Detectors

Worth proposing here since it might require people binding to NEST to make changes. And we'd want to do this once perfectly. @jpbrodsky @mszydagis

@jpbrodsky
Copy link
Collaborator

I'd suggest not moving the G4Integration folder, or at least not calling it an example.

The G4 integration code is not intended to be user-modifiable in the way an example should be. Users of the integration should use the classes provided in their G4 code, rather than trying to copy those classes into their project to modify them. This is because the logic required to pipe G4 tracks through the NEST model is complex and has tons of fiddly cases that break easily.

The G4 integration code is also not a ready-to-run example. It requires an existing G4 Monte Carlo to be added into.

@mszydagis
Copy link
Collaborator

mszydagis commented Oct 2, 2019

@tunnell I have collected solid feedback from everyone. And we would like to make the following modifications to your plan. I have composed an average compromise over everyone's ideas:

Rename Tools to examples: OK

Create src: OK

testNEST.cpp -> Tools/ No, into src instead please.

testSpectra.cpp -> Tools/ Is this a typo?? Did you mean into examples/ since renamed above?

Detectors/DetectorExample_XENON10.hh -> include Not quite: include/Detector

Detectors/VDetector.hh -> include: Again: include/Detector

NEST.cpp -> src: OK

G4integration/ -> examples: @jpbrodsky had said please no, so let's respect that.

Delete empty Detectors -- see above: preserve as its own subdirectory.

One more request, from LZ software expert L. Kreczko: include/*.hh -> include/NEST/ (so, then detector stuff will be in include/Detector in parallel, thus include folder itself will now be empty)

I can speak for LUX and LZ, and if Jason can speak for EXO-200/nEXO and you Chris for XENON1T/nT then all stakeholders have agreed. Please proceed and prepare these changes. Everyone agreed though: never again :) So, it's a case of speak now or forever hold peace..

jecutter added a commit that referenced this issue Nov 14, 2019
…les in src/ folder, header files iin separate Detectors/ and NEST/ folders in the include/ directory, and updated the CMakeLists and README accordingly. G4integration untouched. The project still builds, though Jacob has not checked that optional ROOT tool still compiles as before.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants