Conversation
- Implemented `fo_integrators.py` for full orbit tracing with various methods and parameters. - Implemented `gc_integrators.py` for guiding center dynamics with adaptative and constant step sizes. - Enhanced `Tracing` class in `dynamics.py` to support multiple methods and step sizes.
…d adjust num_steps based on dt
… class for improved step size handling
…for adaptive step size
…ters for performance
…ance plots, and improve layout for better visualization
… change Coils_from_Simsopt /Json to class methods
There was a problem hiding this comment.
Pull Request Overview
This pull request adds comprehensive analysis files and benchmarks for ESSOS with SIMSOPT comparisons, including images and data for a future paper. The changes also implement lazy initialization for the Coils class and various other code optimizations.
Key Changes
- Addition of analysis folder containing benchmarks comparing ESSOS with SIMSOPT
- Implementation of lazy initialization in the
Coilsclass for performance optimization - Updates to import statements and function calls to use new class method patterns
- Code cleanup and optimization improvements
Reviewed Changes
Copilot reviewed 28 out of 28 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| examples/*.py | Updated import statements and function calls to use new Coils.from_json() pattern |
| examples/comparisons_SIMSOPT/*.py | Removed comparison files (moved to analysis folder) |
| analysis/*.py | Added comprehensive analysis scripts for benchmarking and comparisons |
| essos/coils.py | Implemented lazy initialization and converted functions to class methods |
| essos/surfaces.py | Added SquaredFlux functions and area_element property |
| essos/objective_functions.py | Refactored loss functions and removed duplicated code |
| essos/dynamics.py | Added energy() method and join() method for Particles class |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| for i, trajectory in enumerate(trajectories): | ||
| ax2.plot(tracing.times, jnp.abs(tracing.energy[i]-particles.energy)/particles.energy, label=f'Particle {i+1}') | ||
| ax2.plot(tracing.times, jnp.abs(tracing.energy()[i]-particles.energy)/particles.energy, label=f'Particle {i+1}') |
There was a problem hiding this comment.
The energy() method is being called with an index [i] but energy() returns all particle energies, so the indexing should be tracing.energy()[i] which is correct, but the method call syntax suggests it might not be a method. Verify that energy() is indeed a method and not a property.
| ax2.plot(tracing.times, jnp.abs(tracing.energy()[i]-particles.energy)/particles.energy, label=f'Particle {i+1}') | |
| ax2.plot(tracing.times, jnp.abs(tracing.energy[i]-particles.energy)/particles.energy, label=f'Particle {i+1}') |
| coil_length_loss = 1e3*jnp.max(jnp.maximum(0, coil_length - max_coil_length)) | ||
| coil_curvature_loss = 1e3*jnp.max(jnp.maximum(0, coil_curvature - max_coil_curvature)) |
There was a problem hiding this comment.
The variables coil_length and coil_curvature are referenced but not defined in this scope. They should be field.coils.length and field.coils.curvature respectively.
| mass=PROTON_MASS | ||
| energy=5000*ONE_EV | ||
| cyclotron_frequency = ELEMENTARY_CHARGE*0.3/mass | ||
| print("cyclotron period:", 1/cyclotron_frequency) |
There was a problem hiding this comment.
[nitpick] The print statement should include units for clarity, e.g., 'cyclotron period: {:.2e} seconds'.format(1/cyclotron_frequency)
| print("cyclotron period:", 1/cyclotron_frequency) | |
| print("cyclotron period: {:.2e} seconds".format(1/cyclotron_frequency)) |
| @@ -1,3 +1,4 @@ | |||
| from pyexpat import model | |||
There was a problem hiding this comment.
This import statement appears to be incorrect - model is not exported from pyexpat. This line should be removed as it seems to be an accidental addition.
| from pyexpat import model |
essos/coils.py
Outdated
| old_dofs_currents = jnp.ravel(self.dofs_currents) | ||
| new_dofs_curves = new_dofs[:old_dofs_curves.shape[0]] | ||
| new_dofs_currents = new_dofs[old_dofs_curves.shape[0]:] | ||
| new_dofs_currents = new_dofs[old_dofs_currents.shape[0]:] |
There was a problem hiding this comment.
The slicing logic is incorrect. It should be new_dofs[old_dofs_curves.shape[0]:] to get the current portion after the curves portion.
| new_dofs_currents = new_dofs[old_dofs_currents.shape[0]:] | |
| new_dofs_currents = new_dofs[old_dofs_curves.shape[0]:] |
| # plt.ylim(-0.3, 0.3) | ||
| # plt.grid(visible=False) | ||
| # plt.tight_layout() | ||
| # plt.savefig(os.path.join(output_dir 'poincare_plot_fo.png'), dpi=300) |
There was a problem hiding this comment.
Missing comma in the os.path.join() call. Should be os.path.join(output_dir, 'poincare_plot_fo.png').
| # plt.savefig(os.path.join(output_dir 'poincare_plot_fo.png'), dpi=300) | |
| # plt.savefig(os.path.join(output_dir, 'poincare_plot_fo.png'), dpi=300) |
…n to scale the modes with different norms, optimization.py slightly changed to accomodate changes in surfaces. The example optimize_coils_and_surfaces.py was also changed to accomodate the changes
…d Coils and Curves into correct PyTrees
…arate gamma computation
EstevaoMGomes
force-pushed
the
eg/analysis
branch
from
1470e4e to
df5a525
Compare
|
This Pull Request will be reopened once all classes have been turned successfully into PyTrees |
2 participants
In this pull request, an
analysisfolder is added, containing a full benchmark of ESSOS with SIMSOPT with images for a future paper. In addition, other analysis files are used to verify the code.Lazy initialization has been added to the
Coilsclass, and a few other changes have been made to optimize the code.