Conversation
|
There was a problem hiding this comment.
Pull request overview
This PR adds comprehensive planning documents for two major epics focused on GPU acceleration and probabilistic particle representations for the particula aerosol simulation library.
Changes:
- Adds Epic E3: Data Representation Refactor enabling GPU backends via NVIDIA Warp and improved extensibility through dataclass-based data containers
- Adds Epic E4: Probabilistic Particle-Resolved Representation combining particle-resolved accuracy with super-droplet computational efficiency
- Updates documentation indices to reference new epics and features
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| adw-docs/dev-plans/features/index.md | Adds E3 feature table with 4 features for data representation refactor |
| adw-docs/dev-plans/features/E3-F4-facade-migration.md | Defines facade pattern for backward compatibility during migration to new data containers |
| adw-docs/dev-plans/features/E3-F3-backend-warp-integration.md | Specifies NVIDIA Warp integration for GPU-accelerated particle simulations with manual transfer control |
| adw-docs/dev-plans/features/E3-F2-gas-data-container.md | Describes GasData container with batch dimension for multi-box CFD simulations |
| adw-docs/dev-plans/features/E3-F1-particle-data-container.md | Defines ParticleData container with batch-aware array fields and computed properties |
| adw-docs/dev-plans/epics/index.md | Updates epic index with E3 and E4, increments next available ID to E5 |
| adw-docs/dev-plans/epics/E4-probabilistic-particle-resolved.md | Comprehensive epic document for probabilistic particle representation with correlated sampling |
| adw-docs/dev-plans/epics/E3-data-representation-refactor.md | Master epic document describing architecture, scope, and design decisions for data representation refactor |
| adw-docs/dev-plans/README.md | Updates main README with summaries and links for E3 and E4 epics |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ): | ||
| warnings.warn( | ||
| "ParticleRepresentation is deprecated. Use ParticleData instead. " | ||
| "See migration guide: https://particula.readthedocs.io/migration", |
There was a problem hiding this comment.
The migration guide URL "https://particula.readthedocs.io/migration" is hardcoded in the deprecation warning, but this document hasn't been created yet according to phase E3-F4-P5. The warning will show a broken link until the documentation is complete. Consider using a more general documentation URL or adding a note that this URL will be available after E3-F4-P5 completion.
| "See migration guide: https://particula.readthedocs.io/migration", | |
| "See documentation at https://particula.readthedocs.io/ " | |
| "(migration guide URL will be available after E3-F4-P5 completion).", |
| return_legacy = True | ||
| else: | ||
| particle_data = particle | ||
| return_legacy = False | ||
|
|
||
| if isinstance(gas_species, GasSpecies): | ||
| gas_data = gas_data_from_species(gas_species) | ||
| else: | ||
| gas_data = gas_species | ||
|
|
||
| # Perform computation on new data types | ||
| new_particle_data, new_gas_data = self._step_impl( | ||
| particle_data, gas_data, temperature, pressure, time_step | ||
| ) | ||
|
|
||
| # Convert back if needed | ||
| if return_legacy: | ||
| # Update facade's internal data | ||
| particle._data = new_particle_data | ||
| # Update gas species | ||
| gas_species.concentration = new_gas_data.concentration | ||
| return particle, gas_species | ||
| else: | ||
| return new_particle_data, new_gas_data |
There was a problem hiding this comment.
The code example mentions converting gas_species via 'gas_data_from_species(gas_species)' on line 208, but then on line 222 it directly mutates 'gas_species.concentration' assuming it's still the legacy type. However, 'gas_data' is the converted variable that should be used. This creates confusion about which variable represents which type and could lead to incorrect implementation.
| return_legacy = True | |
| else: | |
| particle_data = particle | |
| return_legacy = False | |
| if isinstance(gas_species, GasSpecies): | |
| gas_data = gas_data_from_species(gas_species) | |
| else: | |
| gas_data = gas_species | |
| # Perform computation on new data types | |
| new_particle_data, new_gas_data = self._step_impl( | |
| particle_data, gas_data, temperature, pressure, time_step | |
| ) | |
| # Convert back if needed | |
| if return_legacy: | |
| # Update facade's internal data | |
| particle._data = new_particle_data | |
| # Update gas species | |
| gas_species.concentration = new_gas_data.concentration | |
| return particle, gas_species | |
| else: | |
| return new_particle_data, new_gas_data | |
| particle_legacy = True | |
| else: | |
| particle_data = particle | |
| particle_legacy = False | |
| if isinstance(gas_species, GasSpecies): | |
| gas_data = gas_species.data | |
| gas_legacy = True | |
| else: | |
| gas_data = gas_species | |
| gas_legacy = False | |
| # Perform computation on new data types | |
| new_particle_data, new_gas_data = self._step_impl( | |
| particle_data, gas_data, temperature, pressure, time_step | |
| ) | |
| # Convert back if needed, preserving input types | |
| if particle_legacy: | |
| # Update particle facade's internal data | |
| particle._data = new_particle_data | |
| if gas_legacy: | |
| # Update gas species facade's internal data | |
| gas_species._data = new_gas_data | |
| # Return values with the same legacy/new status as the inputs | |
| if particle_legacy and gas_legacy: | |
| return particle, gas_species | |
| if particle_legacy and not gas_legacy: | |
| return particle, new_gas_data | |
| if not particle_legacy and gas_legacy: | |
| return new_particle_data, gas_species | |
| return new_particle_data, new_gas_data |
| @contextmanager | ||
| def gpu_context( | ||
| data: ParticleData, | ||
| device: str = "cuda", | ||
| ): | ||
| """Context manager for scoped GPU-resident simulation. | ||
|
|
||
| Transfers data to GPU on entry. User is responsible for calling | ||
| from_warp() when ready to transfer back (typically inside the context | ||
| or on exit). | ||
|
|
||
| This is a convenience wrapper - for complex workflows, use | ||
| to_warp()/from_warp() directly. | ||
|
|
||
| Args: | ||
| data: CPU-side ParticleData | ||
| device: Target GPU device | ||
|
|
||
| Yields: | ||
| WarpParticleData on GPU | ||
|
|
||
| Example: | ||
| with gpu_context(particles) as gpu_data: | ||
| for _ in range(1000): | ||
| gpu_data = physics_step(gpu_data, dt) | ||
|
|
||
| # Transfer back inside context | ||
| result = from_warp(gpu_data) | ||
|
|
||
| # Or keep reference and transfer after | ||
| with gpu_context(particles) as gpu_data: | ||
| for _ in range(1000): | ||
| gpu_data = physics_step(gpu_data, dt) | ||
| result = from_warp(gpu_data) # Still valid | ||
| """ | ||
| gpu_data = to_warp(data, device=device) | ||
| yield gpu_data | ||
| ``` |
There was a problem hiding this comment.
The context manager 'gpu_context' yields gpu_data but doesn't handle cleanup or ensure data is transferred back. The docstring mentions "User is responsible for calling from_warp() when ready" but there's no mechanism to ensure this happens or to warn if the user forgets. Consider adding a warning or automatic cleanup on context exit to prevent data loss.
| """Particle representation with backward-compatible API. | ||
|
|
||
| DEPRECATED: This class is deprecated in favor of ParticleData. | ||
| Use ParticleData for new code. This facade will be removed in v1.0. |
There was a problem hiding this comment.
The docstring states "Use ParticleData for new code. This facade will be removed in v1.0." However, there's no indication of what the current version is or when v1.0 is planned. Consider adding more specific timeline information or referencing a migration timeline document.
| Use ParticleData for new code. This facade will be removed in v1.0. | |
| Use ParticleData for new code. This facade is scheduled for removal in | |
| the first major release (v1.0). For current version information and the | |
| most up-to-date migration timeline, see: | |
| https://particula.readthedocs.io/migration#particle-representation-facade |
| # Convert to new types internally | ||
| if isinstance(particle, ParticleRepresentation): | ||
| particle_data = particle.data | ||
| return_legacy = True | ||
| else: | ||
| particle_data = particle | ||
| return_legacy = False | ||
|
|
||
| if isinstance(gas_species, GasSpecies): | ||
| gas_data = gas_data_from_species(gas_species) | ||
| else: | ||
| gas_data = gas_species |
There was a problem hiding this comment.
The variable 'return_legacy' is set based on the particle type check but never used for the gas_species conversion. The code should also check the gas_species type and use it consistently when deciding whether to return legacy types. This could lead to type mismatches where particle type dictates return type but gas type is ignored.
| name: list[str] | ||
| molar_mass: NDArray[np.float64] | ||
| concentration: NDArray[np.float64] | ||
| partitioning: NDArray[np.bool_] |
There was a problem hiding this comment.
The dataclass uses 'partitioning: NDArray[np.bool_]' which might have compatibility issues. In NumPy 2.0+, np.bool_ has been deprecated in favor of np.bool (the built-in bool type). Since the dependency specifies numpy>=2.0.0, consider using 'NDArray[np.bool]' or just 'NDArray' without the dtype specification to avoid deprecation warnings.
| partitioning: NDArray[np.bool_] | |
| partitioning: NDArray[np.bool] |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 participant
add gpu and probilistic paritlces