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

Electron harris #22

Open
wants to merge 13 commits into
base: fsgrid-multipop-marconi
Choose a base branch
from

Conversation

tvbrito
Copy link

@tvbrito tvbrito commented May 29, 2018

General changes to add several features to the Harris project plus adding test population support.

@ykempf
Copy link
Collaborator

ykempf commented May 29, 2018

Some comments from me:

  • Note that the new option for inflow velocity is not only at the boundary as the name would imply, it's in both half-spaces. For the sake of generality it would have been ideal if all components Vxyz could be set separately, but it's not a must now, if needed it's straightforward to add.
  • We have the global option of turning on/off acceleration, translation (and field propagation). Would it make sense to allow to turn acceleration and translation selectively on or off per population, for any population? Because now it is a bit funny that we can turn off acceleration or translation for all populations, test or not, and have a separate switch to turn both off for a test population. It's ok I guess but might lead to confusion in the future.
  • I didn't check thoroughly, there seems to be test-population flags in the relevant places. Now this was tested in proper runs so I guess we'd have noticed any problems. If you feel it would be necessary that I/Urs/Markus go through the Vlasov solver code comprehensively to be doubly sure that we are not by mistake accumulating some test-population moments into the total moments or something similar, let us know.

@ursg
Copy link
Owner

ursg commented May 29, 2018

Otherwise this all looks pretty good to me. Compiles, doesn't break the testpackage, has been tested in the pre-production electron runs.

What would you prefer Thiago? Do you want to still add the separate acceleration nad translation toggles, or should we just merge this and potentially update later?

@ykempf
Copy link
Collaborator

ykempf commented May 29, 2018

Of course it doesn't break the test package, it isn't being tested by it either... But indeed the new options' default values mean that existing cfgs keep working.

@tvbrito
Copy link
Author

tvbrito commented May 29, 2018

  1. True. At the moment, this function is only called for the boundary cells (that's my understanding). But a more specific implementation is better.
  2. I don't know exactly how the acceleration and translation toggles are currently implemented. It may be that the new "propagate" flag is in fact unnecessary. But the "testParticle" flag is definitely a new capability. Maybe what you are saying is that it could have been implemented differently? Yes, maybe. This was done with my limited knowledge of the whole code. Suggestions are very welcome :) It is interesting however, to have these separate capabilities (test particles and propagation) to turn on and off depending on the case.So I would say let's merge these changes while I confirm that the translation toggle is in fact the same as the new flag that I created.
  3. Having more eyes looking at these changes is definitely better.

@ykempf
Copy link
Collaborator

ykempf commented May 29, 2018

  1. No, this function (getV0) is called to initialise every cell.
  2. We can look at it tomorrow (or someone shows you), there is at the top of the cfg usually and in parameters.cpp (I think) flags to turn off acceleration, translation and fields separately (also the Poisson solver). Since we now introduce multipop and test populations I think it would be natural to be able to switch per population a) whether is is a test population, b) whether it should be translated and c) whether it should be accelerated. Of course we can discuss whether this should be done now right away or in the future, whenever it suits us or we need it. I do agree that it will never make much sense to have a self-consistent uranium ion population only being translated while we accelerate lead ions without translating then, while having fully propagated test electrons and self-consistent protons. But if in a test I want to purely translate yet the test population can either be translated+accelerated or neither, I am missing a feature.
  3. I'll have a more thorough look then tomorrow, I have to leave i a few minutes for today.

@ykempf
Copy link
Collaborator

ykempf commented May 29, 2018

And of course if we want 2. now we'll help you. :)

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

Successfully merging this pull request may close these issues.

3 participants