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.

Sign up for GitHub

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

Jump to bottom

[PICMI] Add ionization model configuration support #5007

Draft
wants to merge 19 commits into
base: dev
Choose a base branch
from
Draft

[PICMI] Add ionization model configuration support #5007

wants to merge 19 commits into from

Conversation

BrianMarre
Copy link
Member

@BrianMarre BrianMarre commented Jul 25, 2024 •
edited
Loading

adds the ability to configure ionization model interactions in the PIConGPU-PICMI interface.

replaces PR #4985

  • update documentation
  • add LWFA PICMI script as user example
  • add Test for Ionization configuration
  • verify correct rendering of LWFA with Richard

also fixes a bug in the initialization configuration of the boundElectrons attribute, noticed in the refactoring of the SetBoundElectrons Operation, renamed in this PR to SetChargeState.

@BrianMarre BrianMarre marked this pull request as draft July 25, 2024 18:41
@BrianMarre BrianMarre force-pushed the topic-addIonizerSupport branch 6 times, most recently from e96ac20 to 0b8d2c7 Compare July 29, 2024 08:46
@BrianMarre BrianMarre marked this pull request as ready for review July 29, 2024 08:48
@BrianMarre BrianMarre mentioned this pull request Jul 29, 2024
Closed
@psychocoderHPC psychocoderHPC added this to the 0.8.0 / Next stable milestone Jul 29, 2024
@psychocoderHPC psychocoderHPC added the PICMI pypicongpu and picmi related label Jul 29, 2024
@BrianMarre BrianMarre force-pushed the topic-addIonizerSupport branch 2 times, most recently from 26ef4ab to 1702c17 Compare July 29, 2024 12:42
chillenzer
chillenzer reviewed Jul 30, 2024
View reviewed changes
Copy link
Contributor

@chillenzer chillenzer left a comment

Choose a reason for hiding this comment

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

Hi, just a first batch. Not really touched on the functional code yet.

docs/source/usage/picmi/intro.rst Show resolved Hide resolved
docs/source/usage/picmi/intro.rst Outdated Show resolved Hide resolved
docs/source/usage/picmi/intro.rst Outdated Show resolved Hide resolved
docs/source/usage/picmi/intro.rst Show resolved Hide resolved
docs/source/usage/picmi/intro.rst Outdated Show resolved Hide resolved
lib/python/picongpu/picmi/interaction/__init__.py Outdated Show resolved Hide resolved
lib/python/picongpu/picmi/interaction/ionization/__init__.py Show resolved Hide resolved
lib/python/picongpu/picmi/interaction/ionization/electroniccollisonalequilibrium/thomasfermi.py Outdated Show resolved Hide resolved
lib/python/picongpu/picmi/predefinedparticletypeproperties.py Outdated Show resolved Hide resolved
lib/python/picongpu/picmi/predefinedparticletypeproperties.py
Copy link
Contributor

Choose a reason for hiding this comment

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

Of course, I haven't checked all the numbers, etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

that is fine

chillenzer
chillenzer reviewed Jul 30, 2024
View reviewed changes
Copy link
Contributor

@chillenzer chillenzer left a comment

Choose a reason for hiding this comment

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

Okay. Calling it a day for today. Here's another batch.

General comment: Assertions are not a safe way to check integrity of the data in functional code because they can be deactivated. Such checks should throw other appropriate (potentially custom) forms of exceptions.

lib/python/picongpu/picmi/simulation.py Show resolved Hide resolved
lib/python/picongpu/picmi/simulation.py Show resolved Hide resolved
lib/python/picongpu/picmi/simulation.py
Comment on lines +85 to +90
if picongpu_template_dir is not None:
self.picongpu_template_dir = str(picongpu_template_dir)
else:
self.picongpu_template_dir = picongpu_template_dir
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this not handle a Path directly? Hazarding to state the obvious:Path objects are generally safer to use for paths. If you later need it as a string for plugging it into a template or something, I'd call str() on that as late as possible.

Copy link
Member Author

Choose a reason for hiding this comment

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

because the interface is supposed to be newby friendly, so strings are the default option for now.

lib/python/picongpu/picmi/simulation.py Show resolved Hide resolved
lib/python/picongpu/picmi/species.py Show resolved Hide resolved
lib/python/picongpu/picmi/simulation.py Outdated Show resolved Hide resolved
lib/python/picongpu/picmi/simulation.py Outdated Show resolved Hide resolved
lib/python/picongpu/picmi/simulation.py Outdated Show resolved Hide resolved
lib/python/picongpu/picmi/simulation.py Outdated Show resolved Hide resolved
lib/python/picongpu/picmi/species.py Outdated Show resolved Hide resolved
@mafshari64
Copy link
Contributor

I reviewed the contents for PICMI collisional ionization.

mafshari64
mafshari64 reviewed Jul 31, 2024
View reviewed changes
Copy link
Contributor

@mafshari64 mafshari64 left a comment

Choose a reason for hiding this comment

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

I reviewed the content

mafshari64
mafshari64 reviewed Jul 31, 2024
View reviewed changes
Copy link
Contributor

@mafshari64 mafshari64 left a comment

Choose a reason for hiding this comment

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

I reviewed the content.

@BrianMarre BrianMarre force-pushed the topic-addIonizerSupport branch 2 times, most recently from 1bbfe4b to 78d1c61 Compare August 3, 2024 17:45
@BrianMarre BrianMarre mentioned this pull request Aug 3, 2024
Closed
@BrianMarre
Copy link
Member Author

BrianMarre commented Aug 23, 2024 •
edited
Loading

One CI job failed to acess the github repository, @PrometheusPi coulde you please restart it?

@BrianMarre BrianMarre force-pushed the topic-addIonizerSupport branch 2 times, most recently from 9b56e5c to a9c3e4b Compare August 26, 2024 09:35
@chillenzer
Copy link
Contributor

@BrianMarre @PrometheusPi What's the status of this?

@BrianMarre
Copy link
Member Author

Richard is still in the process of doing an intial review, then I will split up the PR in several independent PRs. At least that is the current plan

@chillenzer
Copy link
Contributor

Thanks for the update. I'll keep quiet then.

@BrianMarre
Copy link
Member Author

Well ... since you ask, you could take over part of the review. ;)

@psychocoderHPC psychocoderHPC marked this pull request as draft September 10, 2024 20:23
@psychocoderHPC
Copy link
Member

I set this PR to draft that it not get merged see: #5007 (comment)

@BrianMarre
Copy link
Member Author

rebased PR against current dev

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mafshari64 mafshari64 mafshari64 approved these changes

@PrometheusPi PrometheusPi PrometheusPi requested changes

@chillenzer chillenzer chillenzer left review comments

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
PICMI pypicongpu and picmi related
Projects
None yet
Milestone
0.8.0 / Next stable
Development

Successfully merging this pull request may close these issues.
5 participants
@BrianMarre @mafshari64 @chillenzer @PrometheusPi @psychocoderHPC