Conversation
b045351 to
7be5b0e
Compare
Prompt + 5-10 follow-ups: > PICMI, which is used as a standard interface in WarpX, is very hard to extend, maintain and develop. > > I would like you to use modern Python 3.10+ and Pydantic 2+ to redesign it, focusing on typing support for checks and very easy extensibility for extra attributes in downstream codes like WarpX. > > First, read all of PICMI in terms of docs and implementation. > > Then, working one class at a time refactor it for using pydantic. > > Make sure the user-facing API stays exactly the same in terms of named classes, attributes per class, and docstrings. > > Godspeed.
| @@ -0,0 +1,13 @@ | |||
| Interactions | |||
| constants | ||
| applied_field | ||
| diagnostics | ||
| interactions |
|
|
||
|
|
||
| class _DocumentedMetaClass(type): | ||
| class _DocumentedMetaClass(ModelMetaclass): |
There was a problem hiding this comment.
I think we should be able to remove all the logic we needed to extend the documentation strings of classes:
Now, all our docs are in the individual fields of a class and the general string of a PICMI class should be able to stay the same in downstream code.
| Base class for all PICMI classes using Pydantic for validation and extensibility. | ||
|
|
||
| This class allows code-specific extensions (e.g., warpx_* kwargs) via Pydantic's | ||
| extra fields mechanism while maintaining type safety for standard attributes. | ||
| """ |
There was a problem hiding this comment.
Not sure if all this mechanism is still needed. I would simply derive in downstream code from a PICMI class and extend it with more fields...
| from .particles import PICMI_Species, PICMI_MultiSpecies | ||
|
|
||
| # Type aliases using Python 3.10+ union syntax | ||
| PICMI_Grid = ( |
There was a problem hiding this comment.
Unused imports from refactoring.
We need to install pre-commits & ruff in PICMI to clean this repo up.
There was a problem hiding this comment.
These type aliases are actually very useful to have. We have them in our implementation as well. But why would you have PICMI_Grid in diagnostics.py?
| # Extract user-defined keywords from extra fields | ||
| if self.model_extra: | ||
| self.user_defined_kw = {} | ||
| keys_to_remove = [] | ||
|
|
||
| # Check density expression | ||
| for k, v in self.model_extra.items(): | ||
| if re.search(rf'\b{k}\b', self.density_expression): | ||
| self.user_defined_kw[k] = v | ||
| keys_to_remove.append(k) | ||
| # Check momentum expressions | ||
| elif self.momentum_expressions[0] is not None and re.search(rf'\b{k}\b', self.momentum_expressions[0]): | ||
| self.user_defined_kw[k] = v |
There was a problem hiding this comment.
See above. Don´t think this is still needed, we can derive in downstream codes and add new fields with their own validators/types.
| # Validate lengths | ||
| if not (lenx == maxlen or lenx == 1): | ||
| raise ValueError("Length of x doesn't match len of others") | ||
| if not (leny == maxlen or leny == 1): | ||
| raise ValueError("Length of y doesn't match len of others") | ||
| if not (lenz == maxlen or lenz == 1): | ||
| raise ValueError("Length of z doesn't match len of others") | ||
| if not (lenux == maxlen or lenux == 1): | ||
| raise ValueError("Length of ux doesn't match len of others") |
There was a problem hiding this comment.
Remove this -- can be checked by type.
chillenzer
left a comment
There was a problem hiding this comment.
Hi,
Hope you're all having a splendid start into 2026!
I'm the new lead developer on PIConGPU's PICMI support. PIConGPU's currently investing quite a bit into its PICMI support and the general direction of this PR suits us very well! We'd be interested in collaborating more closely on the evolution of PICMI, particularly as we have some capacity to do so at the moment. Would you care for a more detailed chat on this via VC?
My two pennies worth of comments you'll find below (just skimmed through, not a full review by far!). On the matter of tearing down infrastructure like _DocumentedMetaClass and _ClassWithInit, I'd agree that this is likely something that Pydantic can easily do for us and that removing them would make implementing PICMI for a particular code more "pythonic" and less based on custom conventions.
Hoping to hear from you soon!
Best,
Julian
|
|
||
| codename = None | ||
| from pydantic import BaseModel, ConfigDict, Field, PrivateAttr, model_validator | ||
| from pydantic._internal._model_construction import ModelMetaclass |
| from .particles import PICMI_Species, PICMI_MultiSpecies | ||
|
|
||
| # Type aliases using Python 3.10+ union syntax | ||
| PICMI_Grid = ( |
There was a problem hiding this comment.
These type aliases are actually very useful to have. We have them in our implementation as well. But why would you have PICMI_Grid in diagnostics.py?
| self.name = name | ||
|
|
||
| self.handle_init(kw) | ||
| grid: "PICMI_Cartesian1DGrid | PICMI_Cartesian2DGrid | PICMI_Cartesian3DGrid | PICMI_CylindricalGrid" = Field(description="Grid object for the diagnostic") |
There was a problem hiding this comment.
This is probably where PICMI_Grid was supposed to be used. More instances below.
| PICMI_Grid = ( | ||
| "PICMI_Cartesian1DGrid | PICMI_Cartesian2DGrid | " | ||
| "PICMI_Cartesian3DGrid | PICMI_CylindricalGrid" | ||
| ) |
There was a problem hiding this comment.
I'm typically inclined to arrange things to avoid forward references. What's the argument for not starting with the grids and defining this alias after all grids are defined?
| self.handle_init(kw) | ||
| methods_list: list[str] = ['FFT', 'Multigrid'] | ||
|
|
||
| grid: "PICMI_Cartesian1DGrid | PICMI_Cartesian2DGrid | PICMI_Cartesian3DGrid | PICMI_CylindricalGrid" = Field(description="Grid instance. Grid object for the diagnostic") |
There was a problem hiding this comment.
We'd probably want to use the alias defined above.
| from .interactions import PICMI_FieldIonization | ||
|
|
||
| # Type unions for PICMI objects using Python 3.10+ union syntax | ||
| # (Note: Not used in type hints anymore, but kept for backwards compatibility) |
|
Hi @chillenzer , Happy New Year! I am out of office today, but happy to meet in the coming week(s)! I only marked it in the PR commit so far, but this draft is just an initial vibe code without a lot of manual improvements yet. So don't spend too much time reviewing it just yet pls :) Yes, let us coordinate and improve the in implementation to be more modern and pedantic! |
|
@chillenzer do you like to take this branch, pull it in your fork, open a new PR from it? |
|
Continued in #133 |
|
@chillenzer oh wait, are you sure #133 was built on this branch? #133 has way less lines changed so far. |
Use a declarative approach to PICMI. Ensure construction & assignment to parameters both trigger validations. Ensure easier extensibility in downstream codes. Enable serialization, so people can easily query their PICMI options, e.g., in post-processing. Close #94