-
Notifications
You must be signed in to change notification settings - Fork 225
Flag for proton and neutron number #687
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
Flag for proton and neutron number #687
Conversation
|
Happy dismembering 🔪 |
|
very attractive, I'll have a look 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@see ... at the next line pls
|
I like it already very much. the mass/charge refactoring might be a good idea for a follow-up pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any future case were we can have only NumberOfProtons or NumberOfNeutrons?
If not and every time both numbers must be set than it might be better to join both flags to one.
e.g.
alias coreElements;
struct Helium
{
static const float_X protons = 2.0;
static const float_x neutrons = 2.0;
};
//add `coreElements` to the flag list
bmpl::vector<
coreElements<Helium>,
ionizer<particles::ionization::BSI<PIC_Electrons> >
> ParticleFlagsIons;There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
absolutely, that's what I thought, too.
that's why I mentioned the same for charge and mass.
since they are const, setting neutr/prot. to zero is also still an
option. (e.g. for protons or pure neutrons)
Axel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would actually even shuffle it more up and add charge / mass directly.
The only thing we have to think about: how to put them transparently, that species 1 can have "protons" fixed but species2 uses "protons", cloned from species 1, as a non-const attribute?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
With a big-ass lookup table one would just give the identifier of the element to the flag list of the species and by bmpl::deselect<vector...< > > one could specify which attributes not to take as const from the table.
The user would just take the basic data of everything from the table ... or set it manually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, basically the deselect must be able to work for const -> non-const attributes.
the "element" name was just given as an example, I think our users should be able to read periodic tables to set helium1 and helium2 in there simulation (or something totally different) :) of course, we can write a nice list of examples in our wiki or elsewhere, too :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just to clearify: "coreElements" is not an ideal name. we should just provide an environment to build what every type of particle/atom a user wants. the user should be able to call it however they like (e.g., they shall be able to create a neutron species, a tripple proton-no neutron atom and neutrinos).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
coreElements is only a example. But we need a fixed name that all algorithms can access this flags to calculate mass and/or charge.
coreElements is only an alias like pusher,ionizer,... and must defined in speciesAttributes.param. It can not changed by the user and must well named by the developer of PIConGPU.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep agreed. in that case atomicNumbers would be a good wrapper name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For charge and mass it is not so easy than for proton numbers. Both are in SI unit and must transformed to pic units :-( We should wait that we can use C++11.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
Again, it compiles but who knows if it runs 😛 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is ionization always enabled in the laser wake field example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, @ax3l told me to remove the pragma before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did I? o.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If someone does not want ionization, they can just remove the ionizer flag. But I don't know if this is too inconvenient right now for the current state of ionization.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, what I meant with that was: we do not need a "general" ENABLE_IONIZATION flag since we will just add something like ionizer<particles::ionization::BSI<PIC_Electrons> > (as it is now).
therefore you just need a PARAM_IONIZATION in the single example that is modified to enable/disable the test.
in short: there should be a PARAM_IONIZATION used in examples/LaserWakefield/include/simulation_defines/param/speciesDefinition.param but none in the other files. also, please use the #if(defined ...) statements I showed you earlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I just still had that in mind somehow and didn't look at the file path. My bad .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great! just remove the last line pls :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why ENABLE_IONS but no ENABLE_IONIZATION ? don't see the difference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
ok :) very nice change set :) |
04f6905 to
02b1a0d
Compare
The flag combines proton and neutron number and by using the trait those can be accessed.
The pre-compiler define `PARAM_IONIZATION` has been added. In `speciesDefinition.param` the flag `atomicNumbers` has been added to the list of ion flags and Helium is used as ion species.
02b1a0d to
9ddabff
Compare
|
Run test also successful 🎐 |
|
Oh ... whoops, I just noticed, you meant one commit, @ax3l . |
|
still ok, thank you! |
|
great and slick pull request 👍 |
Flag for proton and neutron number
Here is another topic concerning multiple species and ionization.
This PR adds a _flag for proton and neutron number_ of the ion species.
In detail:
flagsprotonNumberandneutronNumberatomicNumberstraitsGetProtonNumberandGetNeutronNumberGetAtomicNumbers(this contains a
static_assertwhich is thrown if someone tries to get these numbers from, say, electrons)chargeState=protonNumberionization is now always active if ions are enabled (deactivate by deleting the flags)### The last commit has not been runtime-tested due to the current maintenance(everything until Make proton/neutronNumber a float is safe)