-
Notifications
You must be signed in to change notification settings - Fork 29
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
Variable multipole #510
Variable multipole #510
Conversation
Below an python example script
|
Same for matlab
|
I have tried to include all the feature that were discussed in #503 , but of course this is all up for this discussion. |
There is a problem in the Matlab element creation: atvariablemultipole('ACM','AmplitudeB',1.e-4,'FrequencyB',1.e3); works, but atvariablemultipole('ACM','SINE','AmplitudeB',1.e-4,'FrequencyB',1.e3); crashes. I'll commit a modified version which decomposes |
|
||
for(i=0;i<maxorder+1;i++){ | ||
pola[i]=get_pol(ElemA, ramps, mode, t, turn, seed, i); | ||
polb[i]=get_pol(ElemB, ramps, mode, t, turn, seed, i); | ||
}; | ||
|
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.
This loop rewrites completely the PolynomA/B
fields of the element. It is not recommended for a passmethod to modify its input. Can't we allocate locally pola
and polb
(atMalloc
/atFree
) rather than using the element data fields? Then PolynomA
and PolynomB
can be removed the element fields.
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 did implement it as you suggest to start but them re-introduce PolynomA/B because it was useful to keep track of their values for debugging.
I thought that users may find it useful as well to have access to these values so I left them in. They can be remove of course.
Is there a reason why a passmethod cannot change its element fields? We have several such passmethods for collective effects.
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, no problem. The only danger, and that's why the element attributes should better be "read-only" in the integrator, is that if a user modifies (shortens, removes…) these attributes, the integrator may crash… Since PolynomA
and PolynomB
are not documented, there's a small risk. We can live with that !
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.
How do you handle this in standard elements. if MaxOrder>length(PolynomA/B)
is crashes too right?
I do not see clearly how the Also, is it possible to define a single pulse (which does not repeat as ARBITRARY does) ? |
There are 3 different implementations of the gaussian random generator:
It would be nice to have a single implementation, in an |
Ramp is a multiplicator that allows to set the start and end of the excitation as well as linear ramps up and down. |
I fully agree, and in fact there is a general problem with random number generation as you pointed out in #502 that we need to solve. I would suggest to have a full review in a separate PR |
Why do you say they are not used? |
Ok, I realised later how it works. May be it is worth a little more documentation? |
Because I didn't look carefully enough… Sorry |
Hi @swhite2401 ! Concerning the random generator; I fully agree that the global question of random generation with parallel processing should be addressed in a dedicated PR. For now, my proposal is:
|
Hello @lfarv ! I will do what you propose and include an additional C file to host random generators (don't forget the poisson random number generator as well!) in this PR, then we can merge as is. |
Hi there, This is a very helpful implementation for us, I have been using the branch and all seems OK to me. I just have a couple of small comments:
Cheers, |
Hi Zeus, |
With t2 = t1+1 |
Ah, OK that should work, although it is not completely obvious to me from the help section as it is, I think other users may find it helpful if this behavior is explicitly described. |
Correct, then I will add another optional argument |
Back to the random generator: there is a very simple solution for OpenMP. It consists in replacing double erand48(unsigned short xsubi[3]); This function stores its state not in an internal buffer like
These function are declared in <stdlib.h> and should be standard. We have to check that they are available on all platforms, but if it's ok, they would be the best source for the "common" random generator. It does not solve the MPI problem, though the idea of one buffer per thread with a different initial value is an idea… |
Hello, I have move the random number generator to a separate file Any other comments or suggestions? |
Sounds good, but don't you think that implementing the same method for MPI and OpenMP would be simpler? We can use the rank for both methods |
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.
Looks ok for me
I think that a random generator with an external state variable can solve the problem for both OpenMP and MPI, but possibly not in the same way… 1st problem:
|
Ok let's continue this on a dedicated PR, you initiate it? |
Good, can I merge? @ZeusMarti any other suggestions on your side? |
It is all good from my side, thanks! |
This PR is a follow-up of #503.
It introduces the possibility to have time dependent multipoles using 3 different excitation modes:
A new passmethod was added together with element creation for both matlab and python.