Conversation
henrikjacobsenfys
added
[scope] enhancement
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #95 +/- ##
==========================================
Coverage ? 97.75%
==========================================
Files ? 31
Lines ? 1689
Branches ? 274
==========================================
Hits ? 1651
Misses ? 21
Partials ? 17
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rozyczko
left a comment
rozyczko
left a comment
There was a problem hiding this comment.
Good code. Only typos and minor issues found.
| @@ -1,17 +1,11 @@ | |||
| # SPDX-FileCopyrightText: 2025-2026 EasyDynamics contributors <https://github.com/easyscience> | |||
| # SPDX-License-Identifier: BSD-3-Clause | |||
|
|
|||
There was a problem hiding this comment.
Any particular reason you deleted the license header?
There was a problem hiding this comment.
Copy/paste from old code going wrong
| def create_component_collections( | ||
| self, | ||
| Q: Q_type, | ||
| component_display_name: str = 'Brownian translational diffusion', |
There was a problem hiding this comment.
This should probably say "Jump translational diffusion"
| """Create ComponentCollection components for the Brownian. | ||
|
|
||
| translational diffusion model at given Q values. Args: | ||
| ---------- Q : Number, list, or np.ndarray | ||
| Scattering vector values. | ||
| component_display_name : str | ||
| Name of the Brownian Diffusion Lorentzian component. | ||
| Returns | ||
| ------- | ||
| List[ComponentCollection] | ||
| List of ComponentCollections with Brownian Diffusion | ||
| Lorentzian components. | ||
| """ |
There was a problem hiding this comment.
same here s/Brownian/translational/g
| unit_conversion_factor_nominator = ( | ||
| self._hbar * self.diffusion_coefficient / (self._angstrom**2) |
There was a problem hiding this comment.
nominator -> did you mean numerator?
| angstrom = DescriptorNumber('angstrom', 1e-10, unit='m') | ||
|
|
||
|
|
||
| class TestJumpTranslationalDiffusion: |
There was a problem hiding this comment.
Missing tests for test_scale_setter and test_scale_setter_raises (since they are in the Brownian tests)
There was a problem hiding this comment.
I moved the scal setter and getter to diffusion_base, now the tests are also there
| except Exception as e: | ||
| raise UnitError( | ||
| f'Invalid unit: {unit}. Unit must be a string or scipp Unit and convertible to meV.' # noqa: E501 | ||
| f'Invalid unit: {unit}. Unit must be a string or scipp Unitand convertible to meV.' |
| raise TypeError('Q must be a float.') | ||
|
|
||
| # Q is given as a float, so we need to add the units | ||
| return f'hbar * D* {Q} **2*1/(angstrom**2)/(1 + (D * t* {Q} **2/(angstrom**2)))' |
There was a problem hiding this comment.
Why the extra 1 in the expression?
Just {Q}**2 / (angstrom**2) should be fine
There was a problem hiding this comment.
I think I had problems with it related to the bug with order of operations when dividing parameters
| @@ -62,65 +57,35 @@ def __init__( | |||
| Defaults to "meV". | |||
| scale : float or Parameter, optional | |||
| # ------------------------------------------------------------------ | ||
| # dunder methods | ||
| # ------------------------------------------------------------------ | ||
|
|
There was a problem hiding this comment.
Same for diffusion_coefficient and all other repr I've made so far... I'll streamline them once I know what I want, which is after the Job class :) but it's a good point!
| # ------------------------------------------------------------------ | ||
| # dunder methods | ||
| # ------------------------------------------------------------------ | ||
|
|
There was a problem hiding this comment.
Same for diffusion_coefficient and all other repr I've made so far... I'll streamline them once I know what I want, which is after the Job class :) but it's a good point!
* jump diffusion model * fix notebook * fix weakref? * fix weakref? * fix weakref??? * ... * weakref... * claude fixed it? * respond to PR comments * Move scale test to base model and add some formatting * fix typo * small fix
3 participants
Add Jump Diffusion and remove some unit options that were bugged in the Brownian diffusion. Also had Claude AI fix the weakref bug that had been driving me crazy. It's a temporary solution, but anything is better than getting these random errors that are not related to the code I've written.
Closes #78
closes #66 as irrelevant