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

Feature new primaries #75

Merged
merged 23 commits into from
Mar 22, 2018

Conversation

fmauger
Copy link
Contributor

@fmauger fmauger commented Dec 13, 2017

Preliminary release of the SuperNEMO primary event generator tag 1.3 for MCC2.0.

  • add a new DBD Se82 generator with high-energy range
  • add a flat ranged energy versatile generator with variant support.

Issue:

Additional:

  • this "primaries" setup will be used in the future "simulation/geant4_control" setup tag 3.0.
  • the "resources/config/snemo/demonstrator/simulation/primary_events/1.3/" will be updated with additional
    generators following MCC2.0 requirements.

@cherylepatrick
Copy link

Fantastic, thank you @fmauger !

@drbenmorgan drbenmorgan self-requested a review December 14, 2017 12:20
@drbenmorgan drbenmorgan self-assigned this Dec 14, 2017
@drbenmorgan
Copy link
Member

Also fixes #30.

Copy link
Member

@drbenmorgan drbenmorgan left a comment

Choose a reason for hiding this comment

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

Other than the changes above, we also need:

  • Documentation integrated into the main Doxygen.
  • Basic tests to check that the new generators work.




# end of dbd_more.def
Copy link
Member

Choose a reason for hiding this comment

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

Why does this addition go here, rather than as a new file in primary_events/1.3 like for the flat spectrum generator?

As per #33 I think this additional manually managed versioning (inside a version control system!) needs some justification and documentation.

Copy link
Contributor Author

@fmauger fmauger Jan 18, 2018

Choose a reason for hiding this comment

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

This is not the same concept of versioning than in a VCS.
Here, this resource files "versioning" mechanism is used to "tag" various material that are used to build composite sets of resource files on demand, such sets being also tagged.
This mechanism allows to build a configuration setup by merging different sets of configuration resource files or elementary/atomic resource files.
You can then build higher level setups by picking more fondamental resources from a hierarchy of tagged resources. This has nothing to do with VCS versioning. It is an identification system that allows to navigate through a hierarchy/network of possibly dependant resources, organized in a space of topics, not time.
Of course, care must be taken to assemble together only compatible setups. But this has nothing to do with VCS management. These validation work must be done even without a VCS.
It is what we do when we prepare, as expert developers, a new MC configuration. VCS is not in the loop at this point yet,
but identification of geom/particles/vertex/G4 setup must be relevant.
With this system, it is possible to run in parallel several tagged setups within the same Git version number. Git (or whatever VCS) manages timestamped software versions, while this mechanism manages tagged/identified conceptual setups, independantly of time.
IMO what is confusing is the semantic "X.Y[.Z]" of such tags which looks VCS semantic.
I think I should write a note to explain these concepts that are not easy to address.

Concerning the choice of "common/simulation/primary_events/1.2/generators/dbd_more.def"
of "snemo/demonstrator/simulation/primary_event/1.3" locations, it is completely arbitrary.
Our idea was to consider that:

  • the "Se82.2nubb_2MeV" DBD generator can be used in general independantly of SuperNEMO simulations
    (BiPo...) so it is published as an extension of the existing set of "common" generators.
  • the versatile generator is more specific demand related to the SuperNEMO MC group, it implies to create new variant
    stuff with consequence on the config system and the user interface. Another usage (BiPo...) could have requested
    another set of tweakable params for such generators. So the "snemo" config folder.
    I admit that it is an editorial choice. Historically, Falaise was supposed to be used with other setups like BiPo, commissioning detector... HPGe... It is less relevant now.
    Anyway, the point is to identify any new group of files with a proper unique tag, in such a way it can be freely used, linked, addresses, assembled at higher level without ambiguity.
    HIH
    Frc+Yv'

Copy link
Member

Choose a reason for hiding this comment

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

@fmauger, I think this is actually an argument for VCS! Specifically, it looks to be worthwhile exploring modularisation of the resource files so "tags" can become true Git tags. Creating new top level tags would then be a case of using something like submodules to create an overall bundle tag.

That's not for this PR, but please, contribute this type of info to #33!

root [2] .q
..

.. end
Copy link
Member

Choose a reason for hiding this comment

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

This file should document how to develop and test the resource files, not use them.

Modulo the versions, it may also be better to unify this file with the primary_events/README file to avoid confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not the spirit of this file.
This file is a memo for developpers/experts only. This is not documentation for standard users.
It may give hints to users but stuff in it are not supposed to be used as is.
Such notes helped me to manually test and validate the new config and then
build the test scripts integrated in the build system.

primary_events/README is more for users and its content can be used to
create some documentation.

Copy link
Member

Choose a reason for hiding this comment

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

Then make this explicit in the file! Even for developers, the instructions don't tell you (or link to relevant documentation in Bayeux) what the results of running the various programs should be, or how to develop and test new setups.


exit 0

# end
Copy link
Member

Choose a reason for hiding this comment

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

Document the purpose of this file in the relevant README, including when and how it is supposed to be run.

Copy link
Contributor Author

@fmauger fmauger Jan 18, 2018

Choose a reason for hiding this comment

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

This script is run by the developer of the new config in order to generate a file that lists all available
generators. The resulting CSV file (which is loadable by static variant config) must be considered as a "static" resource file. So I consider it should not be built on the fly by the build system. Maybe it could be but as it is needed during the development stage, I chose to freeze it at an early step. I have not considered possible side effects of such a dynamic approach and I don't want to now.
So it is processed like any other resource files in this directory and must be added in the updated list of resource files.

I agree it will be better to explain when and how it is run while preparing such a new config. Docs, docs, docs!

Copy link
Member

Choose a reason for hiding this comment

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

It feels like the resultant csv file is "compiled" from the input .def files. In other words, it's duplicating information that's already in the .def files, and is a bit like a .o file.

What happens if the developer forgets to regenerate the csv file and commits changes to the def file?

--datatools::resource-path="falaise@$(pwd)/resources" \
--variant-config "@falaise:config/snemo/demonstrator/simulation/primary_events/1.3/variants/repository.conf" \
--variant-gui \
--variant-store "myprofile.conf"
Copy link
Member

Choose a reason for hiding this comment

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

Documentation should go in the main documentation folder, and/or be generated by the build system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. "documentation" is bad name here. Should be "_memos" for example.





Copy link
Member

Choose a reason for hiding this comment

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

As above, if this is a generated file, it should not be under version control, but built (otherwise it'll easily get out of sync). Also, docs should go into documentation.

"@falaise:config/snemo/demonstrator/simulation/primary_events/1.2/generators/misc.def" \
"@falaise:config/snemo/demonstrator/simulation/primary_events/1.2/generators/versatile.def" \
"@falaise:config/snemo/demonstrator/simulation/primary_events/1.3/generators/versatile_more.def"

Copy link
Member

Choose a reason for hiding this comment

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

Why the mix and match of versions here?

gamma.2MeV : Gamma with monokinetic energy @ 2 MeV : Miscellaneous :
gamma.2615keV : Gamma with monokinetic energy @ 2.615 MeV : Miscellaneous :
versatile_generator : Particle with monokinetic energy : User : if_versatile
flat_versatile_generator : Particle with flat energy : User : if_flat_versatile
Copy link
Member

Choose a reason for hiding this comment

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

As above, if this is a generated file, it shouldn't be in version control.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generated files does not systematically mean "dynamically" generated. Some devel tools may be used to automate some products that still have to be considered as static. There can be very good reasons not to dynamically build some material as installation step only. My default approach for resource files is to make things static and not defer the production of such critical stuff to the build system even if it provides very fancy functionalities.
Of course we can consider things case by case but an unique approach is simpler for now.

Concerning this specific file, it can be safely removed. When the final "primary_events/1.3"
will be released (after more addons from MCC2 requests), we will make a new one and push/generate it in the standard documentation repo.

topic.vertexes.component : string = "urn:snemo:demonstrator:simulation:vertexes:4.1"
topic.decays.component : string = "urn:snemo:demonstrator:simulation:decays:1.2"
topic.variants.component : string = "urn:snemo:demonstrator:simulation:2.1:variants"
topic.services.component : string = "urn:snemo:demonstrator:simulation:2.1:services"


# end
Copy link
Member

Choose a reason for hiding this comment

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

Where is decays:1.3 in the final setup?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tag "urn:snemo:demonstrator:simulation:2.1" is built on "decays:1.2" and will be forever.
This should never be changed. The dependancy relationship between tags, once initialized, is frozen.
If you need a new simu setup that used "decays:1.3", then we will create a new tag for that
with new links to tagged modules. The former one will be usabled as before. And in parallel,
the new one is published for users whio requested it.
We have two choices here:

  1. A new tag "urn:snemo:demonstrator:simulation:2.2"
    can be created: it will use the same modules than 2.1 but will link to the new "decays:1.3" in place of "decays:1.2"
    It is a minor update because the decay generator is independant of the rest of the MC setup (geometry/vertex/g4).

This option is immediately feasable. It is just an integration effort in this PR or the next one
after validation of this work.

  1. A new tag "urn:snemo:demonstrator:simulation:3.0"
    will be created in the future: with brand new components:
  • geometry 5.0 (not ready yet, in progress): new realistic source foils, BB materials...
  • vertex 5.0 (not ready yet, in progress, needs geo 5.0) : new vtx generators
  • decay 1.3 (ready, this work)

We believe such work must be split in several PRs because the components
to be addressed are complex and there are strong deps between them. Unit tests
will be easier if we work step by step.
From a new more realistic geometry available for 3D rendering but not MC yet,
users can have new ideas and requests for vtx gnrtors.
Proposal is:

  • 1 PR for a preliminary geom 5.0
  • 1 PR for a preliminary vtx 5.0
  • N PRs for finalization of geom/vtx 5.0/5.0
  • 1 PR for a MC setup 3.0 (integration of former PRs)
  • new Falaise tag may occur here

Copy link
Member

Choose a reason for hiding this comment

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

As per my comment in #82 , this is arguing for splitting the management of resources into separate repos! Not for this PR, but again discussion in #33 is required!

@lemiere
Copy link
Member

lemiere commented Dec 19, 2017

Hi,

MCC2.0 request document asked for new features and we implemented it as quickly as possible.
And we continue to work on many others features.
drbenmorgan asked for changes (mainly documentation or test, is that Yak Shaving ? ) which are time consuming.
So, Is there somebody on SW dev group able to work on requested change by drbenmorgan ?

@emchauve , @yramachers , @cherylepatrick ? @Others ...?
Thanks,

@fmauger
Copy link
Contributor Author

fmauger commented Jan 19, 2018

There is a bug in the resource list file, it includes a broken reference to a local dir.
Have to fix it. Coming soon...

@drbenmorgan drbenmorgan requested a review from lemiere January 19, 2018 15:47
@fmauger
Copy link
Contributor Author

fmauger commented Jan 19, 2018

We have decided to add a new simulation setup tagged 2.2 which enables the two new primaries generators
provided by this work. Associated variant model will be updated.

@fmauger
Copy link
Contributor Author

fmauger commented Jan 19, 2018

Yves, you were right. Don't merge this PR now!
We must PR to fix #79 first. Then the new simu config 2.2 will
use the fixed URN scheme. I'm preparing the PR for #79 now.

@lemiere
Copy link
Member

lemiere commented Jan 19, 2018

Just on time! I was working on it....

@drbenmorgan
Copy link
Member

@fmauger, @lemiere, as #82 is now merged and the feature-new-primaries branch semi-depends on #82, I'd recommend that feature-new-primaries is rebased onto develop, i.e.

... assuming develop is up-to-date with supernemo-dbd/falaise/develop...
$ git checkout feature-new-primaries
$ git rebase develop

This should ensure a logical commit history. The first push back will need the --force flag, but that's fine. Should also make resolving conflicts a bit cleaner.

@fmauger
Copy link
Contributor Author

fmauger commented Feb 13, 2018

There are still fixes to be done in this PR... working on it...

@fmauger fmauger force-pushed the feature-new-primaries branch from 8aedc87 to 6cbfba9 Compare February 13, 2018 17:25
@drbenmorgan
Copy link
Member

@fmauger, that's fine, just let us know when it's ready for final review and merge!

@drbenmorgan
Copy link
Member

@fmauger, any update on the status of this?

@fmauger
Copy link
Contributor Author

fmauger commented Mar 15, 2018

yes, I have found a little time to fix issues in my Bayeux/Falaise setup. Seems to be solved now.
I expect to do a final check of this PR by the end of the week (I'm extremely busy with teaching now).

@cherylepatrick
Copy link

Thank you @fmauger !

@fmauger
Copy link
Contributor Author

fmauger commented Mar 16, 2018

Many residual bugs have been fixed and new functionalities have been consolidated.

Summary:

  1. New primary event generators are:
    • DBD Se82 generator with high-energy range (Esum >2MeV, name="Se82.2nubb_2MeV")
    • flat ranged energy versatile generator with variant support (name="flat_versatile_generator"):
      • particle type can be changed
      • min/max of the flat energy distribution can be changed
    • tweakable single particle generator with variant support (name="tweakable_generator"), extends
      the functionalities of the "flat_versatile_generator":
      • particle type can be changed
      • energy generation mode can be changed:
        • monokinetic (peak energy)
        • flat range (min/max)
        • gaussian (mean+sigma)
        • spectrum from a file (p.d.f. from an ascii file)
    • emission direction mode can be changed:
      • randomized (isotropic)
      • cone (axis + aperture) can be changed
  2. Add new primary event setup tag 1.3.
  3. Add new simulation setup 2.2
  4. Add validation/test script
  5. Update and fix flsimulate documentation

Copy link
Member

@lemiere lemiere left a comment

Choose a reason for hiding this comment

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

Probably useful to change the help with this up to date setup-tag

@drbenmorgan
Copy link
Member

Probably useful to change the help with this up to date setup-tag

Which help do you mean?

@lemiere
Copy link
Member

lemiere commented Mar 22, 2018

@drbenmorgan
`flsimulate-configure --help
[...]
Examples:

$ flsimulate-configure -o myprofile.conf

$ flsimulate-configure
-t "urn:snemo:demonstrator:simulation:2.1"
-o myprofile.conf
[...]
`

I installed that PR#75 using Bayeux 3.1.2 and last brew formula successfully.
It can be merged.

@drbenmorgan
Copy link
Member

O.k., thought you wanted additional changes! Updated base for next release is being prepared, so will now merge in readiness for tha.

@lemiere
Copy link
Member

lemiere commented Mar 22, 2018

Let me know for next release, probably we will do the MCC2.0 using this release.

@drbenmorgan drbenmorgan merged commit 894cf13 into SuperNEMO-DBD:develop Mar 22, 2018
@drbenmorgan
Copy link
Member

@lemiere, @fmauger, just to cross check, the default setup reported by:

$ flsimulate-configure --help
...
Examples:

$ flsimulate-configure -o myprofile.conf

$ flsimulate-configure 
-t "urn:snemo:demonstrator:simulation:2.1" 
-o myprofile.conf
...

comes from this line in FLSimulate:

std::string default_simulation_setup() { return "urn:snemo:demonstrator:simulation:2.1"; }

I assume this should be changed to urn:snemo:demonstrator:simulation:2.2 for the new release? Don't worry about making a PR to do this - I'll do that on the release branch - but need to confirm this is needed.

@fmauger
Copy link
Contributor Author

fmauger commented Mar 23, 2018

I chose not to bump to 2.2 and let 2.1 be the default sim setup in order to let users (re)use their
former work without change. However this may be questionable.
Now people have to explicitly select the 2.2 setup to activate the new functionalities.
So exploring the new functionalities demands (a little) effort to some users without disturbing routine usage by other ones.
It may be decided to make this new setup the official default one (or a few) tag later to let enough time to some people to perform field tests and validate. This can be distinguished from low-level validation: compilation, bug-free released code/configs which is under the responsibilities of the PR process (authors and validators).
I dont know if we should adopt a conventional policy when we publish a new configuration
set like this. Maybe it should be decided on a case by case basis. Feel free to fix/bump
both scripts and docs as you think it better. We will see in the future if a standard approach should be used.

@drbenmorgan
Copy link
Member

Thanks @fmauger, I think as this release is targeting MCC2.0, and this PR addresses the needed requirements of that, we should bump the default to 2.2. I agree though there should be a policy on this!

Generally, I would expect that new setups are tested and validated within PRs, then merged to develop for further testing/validation (develop can be installed/deployed by any user via brew install falaise ---HEAD).

@fmauger
Copy link
Contributor Author

fmauger commented Mar 23, 2018

I'm ok with that approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants