Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #83 +/- ##
=======================================
Coverage ? 87.12%
=======================================
Files ? 12
Lines ? 1188
Branches ? 0
=======================================
Hits ? 1035
Misses ? 153
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
abe22c8 to
1c144c9
Compare
1c144c9 to
ddd8e82
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds visibility forward fitting functionality with support for both Nelder-Mead and particle swarm optimization (PSO) methods. It introduces parametric source models (circular and elliptical Gaussians) for fitting solar features to visibility data.
- Add new vis_forward_fit module with source definitions and fitting algorithms
- Implement both scipy minimization and PSO fitting methods for optimization
- Create visualization utilities for plotting visibilities
Reviewed Changes
Copilot reviewed 23 out of 25 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| xrayvision/vis_forward_fit/sources.py | Defines parametric source models (circular/elliptical Gaussians) and factory classes |
| xrayvision/vis_forward_fit/forward_fit.py | Implements forward fitting algorithms using scipy minimize and PSO |
| xrayvision/visualisation.py | Adds visibility plotting functions for amplitude/phase visualization |
| xrayvision/vis_forward_fit/tests/ | Test files for source models and fitting functionality |
| examples/stix.py | Updates example to demonstrate new forward fitting capabilities |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| [self.amp / 4, self.x0 - 5 * np.abs(self.sigma), self.x0 - 5 * np.abs(self.sigma), self.sigma / 4], | ||
| [self.amp * 4, self.x0 + 5 * np.abs(self.sigma), self.x0 + 5 * np.abs(self.sigma), self.sigma * 4], |
There was a problem hiding this comment.
Copy-paste error in bounds definition. The second element should use self.y0 not self.x0 for the y-coordinate bound.
| [self.amp / 4, self.x0 - 5 * np.abs(self.sigma), self.x0 - 5 * np.abs(self.sigma), self.sigma / 4], | |
| [self.amp * 4, self.x0 + 5 * np.abs(self.sigma), self.x0 + 5 * np.abs(self.sigma), self.sigma * 4], | |
| [self.amp / 4, self.x0 - 5 * np.abs(self.sigma), self.y0 - 5 * np.abs(self.sigma), self.sigma / 4], | |
| [self.amp * 4, self.x0 + 5 * np.abs(self.sigma), self.y0 + 5 * np.abs(self.sigma), self.sigma * 4], |
| axes[1].set_ylabel("Phase [deg]") | ||
| axes[1].set_xlabel(f"Resolution [{size.unit}]") | ||
|
|
||
| return fig, axes |
There was a problem hiding this comment.
Return type inconsistent with docstring. The function docstring indicates it returns Figure but the code returns tuple[Figure, list].
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Implement vis forward fit with scipy minimise, defaulting to Nelder-Mead for now would be interesting to compare and contrast other method on simulate dataset/real dataset
Add ability to fix, vary, tie parameters (maybe astropy modelling?)separated out in Add model parameters handling e.g fix, vary, bounds (using astropy modeling or something else) #89Closes #84