Conversation
…ally, then convert to numpy function for vectorized calculation.
…t from the paper, the sqrt(1-k^2) is actually 1-k^2. Now fix it
…celerate the calcution 4 times faster
2. add english comments to the code 3. fix the singularity problem when x^2+y^2=0 for Bzxx 4. use sympy. to replace sp. To do: There might be another way to convert the sympy expression to a function: autowarp, need to try out.
…o I have sloved a bug in the magnet_cylinder.py. -Now I am currently using 11 cuboids to approximate a cylinder. When viewed from the vertical direction, 11 rectangles are arranged side by side to simulate a circular shape. After testing, the relative error in regions with strong signals is less than 1%, and the runtime has been reduced by nearly 100 times. I have also written the corresponding test files. -I discovered a bug in the old CylinderMagnet class: I had forgotten to account for the offset of the z-coordinate relative to the origin. This issue has now been fixed.
In the polarization function, the prolarization when x=0(relative to the center of the cylinder) is not nan due to we are calcutating 0/0. We used to think there gonna be all nan, however, it turns out to be incorrecte. So now the function will find the yz slice that has nan and replace it with the average of 2 slices new to it. I also added a test for the polarization function. There is another way to fix it, I calcuate the limit of the polarization function when x->0, but that need Bzxx, maybe I can write a new version in the future.
…oxByRectangularMagnet
… returns NaN. Initially, I thought this was due to a 0/0 division in the middle of the x-axis. However, this issue can occur anywhere when the tip-sample separation is too small. Now, in this function, it scans over the x-axis, identifies all NaN values, and replaces them with the average of their neighboring values.
… into FastCermitted
CylinderMagnet.py use analytical solution to calculate the magnetic field, which is too long to calculate, so we decide to just use the approximate method.
The old test rely on CylinderMagnet, which is deleted now
Cylinder magnet
These 2 files are no longger useful
…into CylinderMagnet
Added ``CylinderMangetApprox`` class that approximates a cylindrical magnet. Modified ``rel_dpol_sat_td`` with updated Nan value treatment.
There was a problem hiding this comment.
Pull Request Overview
This pull request adds a new CylinderMagnetApprox class for approximating cylinder magnet magnetic fields, updates the mmodel dependency to version 0.9.0, and improves NaN handling in polarization calculations. The PR also includes extensive documentation improvements and formatting corrections across multiple files.
Key Changes
- Added CylinderMagnetApprox class that approximates cylinder magnets using 11 rectangular magnets
- Updated mmodel dependency from git branch to PyPI version 0.9.0
- Improved NaN value handling in rel_dpol_sat_td to iterate through all NaN values instead of assuming center-only
Reviewed Changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| mrfmsim/component/cylindermagnet.py | New module implementing CylinderMagnetApprox with Bz, Bzx, and Bzxx field calculations |
| tests/test_component/test_cylindermagnet.py | Comprehensive test suite for CylinderMagnetApprox class covering shape, symmetry, and field values |
| mrfmsim/formula/polarization.py | Fixed NaN handling to iterate all NaN indices and updated docstring format |
| tests/test_formula/test_polarization.py | Added test case for NaN handling in rel_dpol_sat_td |
| pyproject.toml | Updated mmodel dependency from git branch to version 0.9.0 |
| docs/contribute.rst | New comprehensive contributing guide |
| mrfmsim/component/init.py | Added CylinderMagnetApprox to exports |
| Multiple documentation files | Various typo corrections, grammar fixes, and documentation formatting improvements |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| CylinderMagnetApprox | ||
| -------------------- | ||
|
|
||
| All sphere magnets are tested using the parameters: |
There was a problem hiding this comment.
The comment says 'sphere magnets' but this test file is for cylinder magnets. This should be 'All cylinder magnets are tested using the parameters:'.
mrfmsim/component/cylindermagnet.py
Outdated
|
|
||
| :param float magnet_radius: cylinder magnet radius [nm] | ||
| :param float magnet_length: cylinder magnet length [nm] | ||
| :param tuple magnet_origin: the position of the magnet origin (x, y, z) |
There was a problem hiding this comment.
The type annotation is inconsistent with other similar parameters in the codebase. In magnet.py line 12, magnet_origin uses list type with math formatting: ':math:(x, y, z) [nm]'. This should be ':param list magnet_origin: the position of the magnet origin :math:(x, y, z) [nm]' for consistency.
| :param tuple magnet_origin: the position of the magnet origin (x, y, z) | |
| :param list magnet_origin: the position of the magnet origin :math:`(x, y, z)` [nm] |
mrfmsim/formula/polarization.py
Outdated
| # adjust the nan values to the average of the surrounding values | ||
| for idx in np.where(np.isnan(div))[0]: | ||
| div[idx] = (div[idx + 1] + div[idx - 1]) / 2 |
There was a problem hiding this comment.
This code has potential index out of bounds errors. If a NaN occurs at the first index (idx=0), accessing div[idx - 1] will fail. Similarly, if a NaN occurs at the last index, accessing div[idx + 1] will fail. Add boundary checks or ensure NaN values cannot occur at edges.
|
Pull request moved to #7 |
Type of Pull Request
Changes Made
Added Cylinder Magnet and minor fixes to docstrings and documentation.
Testing Performed
toxorpytest)Documentation
cd docs && make html)Checklist
Blackformatter's default settings)