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

Allow generation of particles in energy range #29

Conversation

drbenmorgan
Copy link
Member

Discussions on Slack identified a requirement for an event generator in flsimulate that the user could configure with:

  1. Particle type
  2. Lower bound on particle energy
  3. Upper bound on particle energy

Particle energies would be generated uniformly and randomly between the two bounds

Extend flsimulate's primary_events generators and variants with a new "uniform_energy_generator" generator and "if_uniform_energy" variant to meet this requirement. Use genbb's single particle
generator to model the energy range, binding the particle type and min/max energies to variant parameters from if_uniform_energy.

Implement simple run tests of flsimulate to check that the variant can be used and that known failure cases result in exceptions at flsimulate runtime.

One thing for review is that the new uniform_energy_model variant model reuses the existing peg.generator.vspg.energy.PM parameters for the energy. Though the parameter descriptions are set through, e.g. parameters.minimum_energy.description, the GUI only does not display this string. Rather it reuses the description string from the linked model.

Discussions on Slack identified a requirement for an event generator
in flsimulate that the user could configure with:

1. Particle type
2. Lower bound on particle energy
3. Upper bound on particle energy

Particle energies would be generated uniformly and randomly between the
two bounds.

Extend flsimulate's primary_events generators and variants with a
new "uniform_energy_generator" generator and "if_uniform_energy"
variant to meet this requirement. Use genbb's single particle
generator to model the energy range, binding the particle type and
min/max energies to variant parameters from if_uniform_energy.

Implement simple run tests of flsimulate to check that the variant
can be used and that known failure cases result in exceptions at
flsimulate runtime.
@fmauger
Copy link
Contributor

fmauger commented Jun 7, 2017

PR29 review.
A few preliminary comments:

  1. rename the "peg.generator.uniform_energy_model" to "peg.generator.uniform_energy.VM"
    .VM sufffx is for variant models, .PM is for parameter models. This preserves the rules used in the
    peg.def file so far.
  2. to guarantee backward compatibility, a new "primary_events/1.3" should be created to host the updated configuration of the prim. ev. manager, leaving the current "primary_events/1.2" setup and its variant setup unchanged. no need to clone all the files, one can reused some files from 1.2 in 1.3, changing/adding just
    what is necessary.
  3. a new simulation setup 2.2 should be created (the new default one) based on :
  • demonstrator setup "1.0" (current)
  • vertexes "4.1" (current)
  • primary_events "1.3" (new)
  1. add new entries in the "resources/urn" database and path resolver
    to represents this new assembly of dependencies.
  2. the description string of parameters in the variant service GUI is odd. it is not a Falaise issue. It will be fixed in Bayeux soon (Variant service GUI does not display the proper description for parameters BxCppDev/Bayeux#5).

@drbenmorgan
Copy link
Member Author

I've fixed the name suffix - just to confirm, this suffix is purely a convention of the file and not something specific to the variants syntax?

@drbenmorgan
Copy link
Member Author

to guarantee backward compatibility, a new "primary_events/1.3" should be created to host the updated configuration of the prim. ev. manager, leaving the current "primary_events/1.2" setup and its variant setup unchanged. no need to clone all the files, one can reused some files from 1.2 in 1.3, changing/adding just
what is necessary.
a new simulation setup 2.2 should be created (the new default one) based on :
demonstrator setup "1.0" (current)
vertexes "4.1" (current)
primary_events "1.3" (new)
add new entries in the "resources/urn" database and path resolver
to represents this new assembly of dependencies.

Could you describe the method used for backward compatibility here please? The change is easy enough to do, albeit with many steps, so it's a good place to learn about the design and mechanism.

@fmauger
Copy link
Contributor

fmauger commented Jun 7, 2017

Yes, the suffix is purely a convention of the file. Just to help people to understand what they are manipulating. It is obviously redundant with the type="variant" as .PM is with type="parameter".

@fmauger
Copy link
Contributor

fmauger commented Jun 7, 2017

Concerning "backward compatibility", I have to think more about it before to emit a valuable comment
and a strong recommendation to manage the resource files in a long term.

In some cases, modifying the variant grammar (definition of nested variant and parameter models)
should not break an existing variant system.
However, a modification could imply the suppression of some variant parameters from a model, because we finally believe it is nonsense (probably a design bug!) , or there is a better way to achieve some required flexibility (new feature!).
In such case, it would not be possible to replay a previous run using a given blessed profile.
This may be a big problem to reuse existing MC production while the tagged variant profile
used to generate the dataset has changed its internal structure. Adding variant material in a model
could also imply arbitrary inserting of new records in the profile.
The order used to define param instances in variant models and associate variant instances
in param models is important. Swapping two items of such a list makes the system incompatible
with the former version. We thus have to be extremely careful here to avoid entropy.

I think there is 2 cases to consider:

  • a bug fix in the variant grammar/system: we fix the files and we don't change the tag of the setup (URN) the dataset already produced with the setup are marked with a special quality flag by the
    Data Quality group's policy and probably not reused if data corruption is proven.
  • a new feature in the variant grammar/system: adding/removing parameters and associated variant models, publishing new variant and param models. Typically, this changes the layout of the variant profile hierarchy with a strong risk to break the possibility to reuse/understand/replay the existing dataset produced in the variant context. We also have to consider the "long range" deps between variant
    registries (a vertex generators setup depends on the geometry setup).

Here, we are in the second case. I agree the change/addon in variant models def file is not complex. However, the URN management part is more complex. For this part, I have to write recommendations.
Bayeux still misses some helper porcelain API to provide high level functionnalities (graph viz
support for URN tagged setup dependencies, dep path traversing and searching... it is in my todo list).

@drbenmorgan
Copy link
Member Author

Thanks @fmauger! I've created #33 for further discussion/documentation on this (can be cross referenced with Bayeux Issues/PRs as required).

For this PR, I'll go ahead and implement review recommendation to make the new version and updates.

Copy changed files from version 1.2, updating internal links to
1.3 and re-using 1.2 where possible.

Provide new "uniform_range_generator" and associated variants.
Copy files from version 2.1 needed to update primary_events to
version 1.3. Otherwise reuse files from 2.1
Rerun script to regenerate resource list.
A pure hack as the structure of the DB and purpose is unclear.
Completely copied from existing sections, just update numbers as
required.
Make default simulation setup the new versino 2.2 URN.

Remove explicit URN use in tests to ensure these always test against the
default version.
@drbenmorgan
Copy link
Member Author

The new commits should meet the list of additions in the review, though I am not 100% sure about the changes in the URN DB section.

@fmauger
Copy link
Contributor

fmauger commented Jul 13, 2017

Hi there,
I had no time to consider further this PR. Basically the mods are ok but some issues
and side effects should be considered before to expand the set of configuration tags/URNs at the risk of
being unconsistant in a near future. I'll be in vacation from tonight for a while.
However, I have recently worked on the configuration tagging (using URNs) policy also with new documentation on URN tags.
I now consider there is additional work to do about the management of services (and associated configuration files/tags) used by the pipeline(s) (with a similar approach for FlSimulate and FLreconstruct).
Then management of configurations (tagging, versioning, dependencies, backward compatibility... ) will become more clear and automated AFAP, also to avoid entropy. I consider the current system is promising but unperfect and still error prone. I propose to defer this PR with the new generator after additional work has been done both on Bayeux and Falaise. Recent meeting with the AMI group (Atlas database for configuration tags, file indexing and configuration/dataset cross referecning) and discussions with Yves have been very useful but I need time to think about all that stuff.
I have a preliminary support for directed-acyclic dependency graph in upcoming Bayeux and additional tools in my Falaise repo to provide useful tools to address these complex problems. but it needs more work...
cheers

@drbenmorgan
Copy link
Member Author

Then management of configurations (tagging, versioning, dependencies, backward compatibility... ) will become more clear and automated AFAP, also to avoid entropy. I consider the current system is promising but unperfect and still error prone. I propose to defer this PR with the new generator after additional work has been done both on Bayeux and Falaise. Recent meeting with the AMI group (Atlas database for configuration tags, file indexing and configuration/dataset cross referecning) and discussions with Yves have been very useful but I need time to think about all that stuff.
I have a preliminary support for directed-acyclic dependency graph in upcoming Bayeux and additional tools in my Falaise repo to provide useful tools to address these complex problems. but it needs more work...

Great - on the Falaise side, it's fine (and indeed needed in this case I think) to start a "WIP" Pull Request so that the proposed changes can be previewed. It's just the same as a "standard" PR, just add "[WIP]" at the start of the title!

It would also help to see (and cross reference to) the work in Bayeux so that we can see where changes are going to affect Falaise.

@fmauger
Copy link
Contributor

fmauger commented Mar 20, 2018

I guess this PR can be safely discarded fir new PR75 addresses the new requested functionalities
including the URN management system.
Comments ?

@drbenmorgan
Copy link
Member Author

Yes, once the release is finished and we do a sweep up.

@drbenmorgan
Copy link
Member Author

Closing as #75 fixes this issues

@drbenmorgan drbenmorgan deleted the add-energy-range-generator branch March 22, 2018 13:29
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.

2 participants