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

Python: useful error when initialize_warpx not called before creating ParticleContainerWrapper #5412

Merged
merged 8 commits into from
Jan 11, 2025

Conversation

archermarx
Copy link
Contributor

@archermarx archermarx commented Oct 24, 2024

Currently, WarpX is initialized when sim.step is called, or when the user calls initialize_warpx. However, if the user tries to create a ParticleContainerWrapper before this point, they get an error along the following lines:

 File "/home/marksta/projects/warpx-ionization/picmi.py", line 185, in <module>
    sim.run()
  File "/home/marksta/projects/warpx-ionization/picmi.py", line 179, in run
    self.elec_wrapper = particle_containers.ParticleContainerWrapper(self.electrons.name)
  File "/home/marksta/.local/lib/python3.10/site-packages/pywarpx/particle_containers.py", line 29, in __init__
    mypc = libwarpx.warpx.multi_particle_container()
  File "/home/marksta/.local/lib/python3.10/site-packages/pywarpx/_libwarpx.py", line 46, in __getattr__
    return self.__getattribute__(attribute)
AttributeError: 'LibWarpX' object has no attribute 'warpx'

This is confusing. When I got this, I assumed I had maybe installed WarpX wrong. I added a catch for this exception that re-raises it with some additional context that should help direct the user to call initialize_warpx

@RemiLehe RemiLehe self-assigned this Oct 28, 2024
@RemiLehe RemiLehe self-requested a review October 28, 2024 23:03
@archermarx archermarx requested a review from dpgrote November 4, 2024 22:44
@dpgrote
Copy link
Member

dpgrote commented Nov 5, 2024

I would suggest handling this in a different way. Now, the issue is that the libwarpx.warpx is accessed in the __init__ routine. A change would be to only access libwarpx.warpx when it is needed. Perhaps make particle_container a property that gets the underlying particle container when needed. The user can then create the ParticleContainerWrapper at any time. The message this PR adds would then go in the particle_container property get method. This is how the field wrappers are setup.

@archermarx
Copy link
Contributor Author

Got it, thanks for the feedback!

@archermarx
Copy link
Contributor Author

archermarx commented Jan 9, 2025

Ok, I've modified the PR to now lazy initialize the particle container (and boundary buffers) only when they're first needed instead of in __init__. Only then is an error thrown.

@archermarx archermarx assigned archermarx and ax3l and unassigned RemiLehe and archermarx Jan 9, 2025
@archermarx archermarx requested review from ax3l and removed request for RemiLehe January 9, 2025 20:29
@archermarx
Copy link
Contributor Author

Changes made. That's definitely a lot cleaner.

Copy link
Member

@dpgrote dpgrote left a comment

Choose a reason for hiding this comment

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

Thanks! This looks good to me.

@dpgrote dpgrote merged commit 555f435 into ECP-WarpX:development Jan 11, 2025
37 checks passed
self.name
)
except AttributeError as e:
msg = "This is likely caused by attempting to access a ParticleContainerWrapper before initialize_warpx has been called"
Copy link
Member

@ax3l ax3l Jan 16, 2025

Choose a reason for hiding this comment

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

Thank you for this PR! Sorry for just seeing it now, I was just on parental leave when the PR was opened.

I thought we could also add one more fallback, right in the library loader, where we assign the libwarpx.warpx attribute.

In essence, we could assign an initial attribute that throws when accessed with a more distinct error message ("initialize_warpx was not yet called!"), to catch all. More specific exceptions like this one are great to add and keep.

One initialization, we then just overwrite the attribute.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh neat idea! I can open a new PR shortly!

@ax3l ax3l added the component: Python Python layer label Jan 16, 2025
ax3l pushed a commit that referenced this pull request Jan 24, 2025
Follows up on, #5412, using
@ax3l's
[suggestion](#5412 (review))
to catch warpx not being initialized when accessing libwarpx.warpx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Python Python layer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants