Conversation
…ag to pyproject.toml.
…dded more subtests to the Beam initialisation tests.
…proper velocity and momentum are above a given threshold. Also added controls for the number of particles, number of bunches in a train and bunch separation.
…tudinal proper velocity and momentum being above a given threshold. Also some more edts on the rotation tests.
…so minor edits on some docstrings.
…ntum directly as m*u.
…velocity2momentum() in relativity.py where the momentum is now calculated as m*u.
…f tests for beam statistics.
…mittance_y() to prevent negative emittances.
…() to enable using parameter evolution tracking when calling Beam.apply_betatron_motion().
…under statistics.py for when the sum of weights is zero.
…hase_space() for when num_particles is an integer smaller than 1.
…m_transverse_vector(), and test_gamma_xy() to test_beam.py.
…d tests for triggering the exceptions.
|
The |
kyrsjo
left a comment
kyrsjo
left a comment
There was a problem hiding this comment.
Great job with lots of new tests!
One comment however - in general moving lots of code around in an existing file should be avoided unless really needed, since (a) it makes reading the diff very difficult and (b) it can make merges hell. If it's really needed, I guess it's better to do it in it's own pull request, where one can more easily and neatly separate out the real changes from the just-moved-things-around changes.
Approved, with a few small comments that should probably be fixed before merging, but I don't think more input is needed.
I did not run the code, but I trust that all tests pass, not just the ones you've added :)
| # ========== Betatron oscillations ========== | ||
| beam.apply_betatron_motion(self.length_flattop, self.plasma_density, self.nom_energy_gain_flattop, x0_driver=driver0.x_offset(), y0_driver=driver0.y_offset()) | ||
| deltaEs = np.full(len(beam.Es()), self.nom_energy_gain_flattop) | ||
| if self.calc_evolution: |
There was a problem hiding this comment.
Why not pull the beam.apply_betatron_motion outside of the if? Its called by the same parameters anyway, the only difference is whether self.evolution.beam is set or not.
Also, why is it called? Are there side effects to beam when this is called (the function has no docstring)?
There was a problem hiding this comment.
It is inside of if self.calc_evolution because it only tracks the beam parameter evolution when self.calc_evolution=True. And this is the function that actually evolves the beam by solving Hill's equation. I will add docstrings to this (
commit 3a246fa).
| # def proper_velocity2momentum(u): | ||
| # return gamma2momentum(proper_velocity2gamma(u)) | ||
| def proper_velocity2momentum(u, m=SI.m_e): | ||
| return m*u |
There was a problem hiding this comment.
Is test_StagePrtclTransWakeInstability_beamline the name of a test file, and test_baseline_linac a test function within this file?
If not, how are these names derived?
There was a problem hiding this comment.
Yes, it is. This is on another branch. Just using the same file as reference here.
There was a problem hiding this comment.
OK. Hopefully the conflict will resolve itself peacefully, if you've committed the same new file to two branches.
| "Verify that the phase space is set correctly." | ||
| beam = Beam() | ||
| num_particles = 10042 | ||
| xs = np.random.normal(0.0, 2.0e-6, num_particles) |
There was a problem hiding this comment.
Yeah, I really really need to get on with centralising the RNG. That might reduce the need for rtol/atol-hacking.
| beam.set_phase_space(Q, xs, ys, zs, uxs, uys, uzs) | ||
|
|
||
| assert len(beam) == num_particles | ||
| assert np.isclose(beam.particle_mass, SI.m_e, rtol=1e-15, atol=0.0) |
There was a problem hiding this comment.
Is this basically testing whether beam.particle_mass is identical (as it should be)? Is there no better way to do this? Maybe at least wrap in a function like "are_arrays_identical" in a tests/testutils.py file?
tests/test_sources.py
Outdated
| def test_SourceFromFile2Beam(): | ||
| "Check that ``SourceFromFile`` retuns returns the same ``Beam`` as the input ``Beam``." | ||
|
|
||
| beam_file = 'tests/data/test_StagePrtclTransWakeInstability_beamline/test_baseline_linac/shot_000/beam_003_00048.558626.h5' |
There was a problem hiding this comment.
Filenames hardcoded in this way is not great, and could break.
Something like the function getAndMakeTestOutputFolderPath(testname) here is probably useful:
https://gitlab.cern.ch/clic-software/clicopti/-/blob/master/Python/tests/testutils.py?ref_type=heads#L182
There was a problem hiding this comment.
Not sure how to fit in getAndMakeTestOutputFolderPath(testname) here, but I have added controls in source_from_file.py to ensure valid file path in both the constructor and file setter (commit 9bb7a73). Also edited test_SourceFromFile2Beam() in test_sources.py to trigger exceptions for invalid file path (
commit b903b82).
There was a problem hiding this comment.
I had problems reviewing these changes, since they mixed moving around code with changing code.
This caught some of it:
https://github.com/carlandreaslindstrom/ABEL/compare/413fddeda019edd5341369f32c12747399495203..97528904932bde4deec5016df48befadd7e4ba08
(using this syntax for comparing files directly in github: https://docs.github.com/en/pull-requests/committing-changes-to-your-project/viewing-and-comparing-commits/comparing-commits )
|
Also, some more description (edit the topmost comment here) would probably be good - there are changes outside of just adding tests. |
…th the constructor and file setter.
…tions for invalid file path.
This reverts commit 027c598. That commit moved functions around in Beam : filename, save, load
2 participants
Added tests for the
Source(and its subclasses) andBeamclass. Also made changes inrelativity.py,beam.py,stage_basic.pyandstatistics.py.