Closed nabobalis closed 10 months ago
marked this merge request as draft
In GitLab by @codecov on Jul 2, 2022, 21:47
Merging #159 (105e9ef) into main (a4f0ca2) will decrease coverage by
2.26%
. The diff coverage is52.27%
.
@@ Coverage Diff @@
## main #159 +/- ##
==========================================
- Coverage 95.90% 93.63% -2.27%
==========================================
Files 19 20 +1
Lines 610 644 +34
==========================================
+ Hits 585 603 +18
- Misses 25 41 +16
Impacted Files | Coverage Δ | |
---|---|---|
aiapy/data/_sample.py | 56.86% <44.82%> (-7.66%) |
:arrow_down: |
aiapy/calibrate/transform.py | 50.00% <50.00%> (ø) |
|
aiapy/calibrate/__init__.py | 100.00% <100.00%> (ø) |
|
aiapy/calibrate/prep.py | 98.27% <100.00%> (+0.09%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update a4f0ca2...105e9ef. Read the comment docs.
marked the checklist item Deal with any deprecations as completed
marked the checklist item Changelog for use_scipy to method keyword break as completed
marked the checklist item Changelog for sunpy bump as completed
marked the checklist item Changelog for python bump as completed
added 5 commits
marked the checklist item Add cupy support for affine transform as completed
marked this merge request as ready
I am unsure if this can be tested on the CI, it runs locally tho and outputs an image.
In GitLab by @wtbarnes on Jul 18, 2022, 14:31
Commented on aiapy/calibrate/tests/test_prep.py line 220
Yeah can't do any GPU stuff on the CI, which is why that gallery entry is skipped. Can you verify that its actually faster on a GPU?
Locally, its not any faster on the aia 171 test map than scipy. That file is only 128 by 128 if I recall.
On the roll image:
In [12]: amap = sunpy.map.Map(sunpy.data.sample.AIA_171_ROLL_IMAGE)
In [13]: %%timeit
...:
...: register(amap, method="scipy")
...:
...:
2.23 s ± 23 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
In [14]: %%timeit
...:
...: register(amap, method="cupy")
...:
...:
121 ms ± 40.8 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)
(This is using a 3080)
marked the checklist item More? as completed
There is a mistake in my function that I need to track down.
It had the same problem that my 3.1.7 PR broke. This is now fixed.
In GitLab by @wtbarnes on Jul 26, 2022, 07:41
Commented on aiapy/calibrate/transform.py line 9
This needs a test, even if it doesn't run on the CI regularly. We need a way to systematically say whether the scipy and cupy methods give the same result (to within some tolerance).
In GitLab by @wtbarnes on Jul 26, 2022, 07:44
Commented on aiapy/calibrate/tests/test_prep.py line 220
ok yeah so it's ~20x faster than just plain scipy. I would only expect significant gains on the full-res, full-frame images so the fact that it's not any faster on the test map isn't surprising.
In GitLab by @wtbarnes on Jul 26, 2022, 07:45
Commented on aiapy/calibrate/transform.py line 9
We probably should have the same for the PSF and deconvolution calculations as well.
I can write them, but will they be that similar?
Do we need that comparison for the PSF and deconvolution calculations?
In GitLab by @wtbarnes on Jul 26, 2022, 10:38
Commented on aiapy/calibrate/transform.py line 9
The PSF comparison can wait actually. I think #75 already tracks that.
Re: the register comparison, I think if we are going to support an alternate rotate method specifically for register
, we should be checking that it agrees with the default implementation to within some reasonable tolerance. Additionally, my understanding is that the cupy version is an implementation of the scipy affine transform so they should agree.
I added an assert test, it passes locally. See if you like it.
In GitLab by @wtbarnes on Jul 26, 2022, 14:50
Commented on aiapy/calibrate/transform.py line 9
:thumbsup:
In GitLab by @wtbarnes on Jul 26, 2022, 14:50
resolved all threads
In GitLab by @wtbarnes on Jul 26, 2022, 14:52
approved this merge request
In GitLab by @wtbarnes on Jul 26, 2022, 14:53
This looks good to me, but what's with the failing online test? It looks like it can't grab the sample data.
Let me check, we might need to do some changes to the code like sunpy had to do for parfive.
I updated the URL, that should fix it.
approved this merge request
Merges ci -> main
TODO