Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#1265 quinticje #1266

Closed

Conversation

jecampagne
Copy link

Hello,
This PR is related to issue #1265 where I've announced a modified version of the default GalSim Quintic filter/kernel (Bernstein & Gruen 2014 paper). This new 5th-order filter is called "QuinticBis" in the PR and $K^{je}_5$ is the table below which is an update of Table 1 of the paper.
image.

The files modified are

  1. in src: Interpolant.cpp
  2. in include: Interpolant.h
  3. pysrc: Interpolant.cpp
  4. interpolant.py
    to create de "QuinticBis" class following the Quintic one.

For the unit test, I have adapted the following functons in test_interpolatedimage.py

  1. test_interpolant
  2. test_unit_integral
  3. test_realspace_conv
  4. test_conserve_dc
  5. test_ii_shoots
  6. test_quintic_glagu
  7. test_depixelize
    and create test_QuinticBis_ref. One point: as Quintic is the default kernel for InterpolatedImage, I have NOT change this default, so many of the tests do not use "QuinticBis".

I have followed the README workflow, although I am not sure that PRs from non-official-GalSim-developper are really envisaged. Hope that I've not missed some important x-checks.

Let me know what do you think. Thanks

@@ -30,16 +39,19 @@
from .integ import int1d
from .bessel import si


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the whitespace changes, which are unconnected to your new code.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it is due Black formatting that I have activated in VSC. Ok I will remove them.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact Black had changed several formatting features. Try to "deBlack" ...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hope that it's ok now. I have verify that the tests are passed.

Copy link
Member

@rmjarvis rmjarvis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly, I'm not sure I understand what exactly is better about this compared to the existing Quintic interpolant. So I'd like to see some explanation of what the pros and cons are of the two options to give the user some guidance about whether they want to switch. I think the best place to investigate this is probably the existing unit tests of sheared InterpolatedImage profiles, since that was what Gary was trying to optimize with the Quintic.

You can also look at an old scripts here: https://github.com/GalSim-developers/GalSim/blob/releases/2.5/devel/modules/test_interpolants.py and https://github.com/GalSim-developers/GalSim/blob/releases/2.5/devel/modules/test_interpolants_parametric.py

I'm sure they are quite bitrotted by now, since they are quite old, but they might be something you can refresh to use the current API. They compare the accuracy of shear and shifted InterpolatedImages with various interpolants, so it could be interesting to add in your new interpolant to see how it compares in real world use cases.

galsim/interpolant.py Outdated Show resolved Hide resolved
galsim/interpolant.py Outdated Show resolved Hide resolved
@@ -601,6 +624,78 @@ def krange(self):
return 1.6058208066649935 / self._gsparams.kvalue_accuracy**(1./3.)


class QuinticBis(Interpolant):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'Why Bis? "Twice in Latin I suppose? But I'm not sure that twice Quintic is an apt descriptor for this class.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well you are right "Bis" is for distinguishing the 1st and 2nd houses on a road for mail address which share the same number: eg. 1, 1Bis, 1Ter...
Now, I'd no inspiration to get a better naming: wanted to tell that its a Quintic kernel with the same complexity, but I miss some tag to tell about its differences with Gary's one.

class QuinticBis(Interpolant):
"""Fifth order interpolation

The quintic interpolant is an improved version over the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure that this is "improved" for all use cases? I think it just has slightly different properties, right?

Better would be to explain how it differs from the original Quintic, and what the pros and cons are, if you know.

Related to this, it would be worth trying out the Quinticbis interpolant with some of the InterpolatedImage shear tests to see how the accuracy compares in those cases. Specifically the functions test_operations and test_operations_simple in test_interpolated_image.py. If you want to claim that the new interpolant is better in some way, then I think you should try to show that the shear accuracy is improved when using this class for kvalue_interpolant.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree "improved" is a always wrt some point of view.
I will answer to what are the differences.

Copy link
Author

@jecampagne jecampagne Dec 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are some explanations how I've found the expression of the QuinticBis kernel.
Let us define a piecewise polyniomal kernel as followed

$$K(x) = \left\{ \begin{array}{ll} \displaystyle \sum_{j=0}^n a_{ ji } |x|^j & i \leq |x| < i+1 \\\ 0 & |x| \geq m \end{array} \right.$$

with $n=2m-1$ (m=2 => cubic, m=3 => quintic). There are $2m^2$ coefficients.

Here are the constraints that one can consider in the real-space:

$$(1) \quad K(j) = \delta^K_{j0},\ \forall j\in \{0..m-1\}$$ $$(2) \quad a_{\ell,0}=0, \quad \ell \in \{1..m-1\}\ \mathrm{and} \ \ell \ \mathrm{odd}$$

and for $m \geq 3$:

$$(3) \quad \lim_{x\rightarrow j^{-}}K^{(\ell)}(x) = \lim_{x\rightarrow j^{+}}K^{(\ell)}(x), \forall j\in \{1..m\} \ \mathrm{and}\ \forall \ell \in \{0..n-4\}$$

Now, considering constraints in the Fourier space:

$$(4)\quad \widehat{K}(0)=1$$ $$(5)\quad \widehat{K}^{(\ell)}(0) = 0, \forall \ell\in \{2..n\} \ \mathrm{and}\ \ \ell \ \mathrm{even}$$ $$(6)\quad \widehat{K}^{(\ell)}(1) = 0, \forall \ell\in \{0..n-1\}$$

It turns out that one recover for $m=2$ the usual cubic kernel, there is also a septic kernel (not considered later). Concerning the Quintic case, ie. $m=3$, there is a coefficient left non determined (a degree of freedom), one can choose $a_{20}$ for instance. How to choose this coeff?

Well, the Taylor development of $K(x)$ at $x=0$ leads to

$$K(x) = 1 + a_{20} x^2 + O(x^3)$$

If one chooses $a_{20}=0$ to "flatten" the behaviour around x=0 then one recovers Gary's quintic kernel (nicknamed hereafter $K^G$). With such definition

$$K^{G}(x)~\frac{1}{12} |x|^3 + O(x^4)$$

while concerning the Fourier space

$$\begin{array}{rcl} \widehat{K}^G(u) &=& 1 - \frac{25 \pi^6}{189} u^6 + O(u^7) \\\ \widehat{K}^G(u) &=& 2(\pi^2-27) (u-1)^5 + O((u-1)^6) \end{array}$$

Looking at the difference of behaviours around $u=0$ and $u=1$, and remembering the law that governs the 1st ghost intensity, I've made an another choice:

$$a_{2,0}\to -\frac{5 \left(27-\pi ^2\right)}{4 \left(15-\pi ^2\right)}$$

leading to

$$\begin{array}{rcl} \widehat{K}^{JE}(u) &=& 1-\frac{\left(\pi ^6 \left(143 \pi ^2-2361\right)\right) }{945 \left(\pi ^2-15\right)}u^6+O\left(u^7\right)\\\ \widehat{K}^{JE}(u) &=& \left(-71-7 \pi ^2-\frac{1080}{\pi ^2-15}\right) (u-1)^6+O\left((u-1)^7\right) \end{array}$$

So, to summarise

  1. Both kernels share the "quintic" property of iexact interpolation of fourth-order polynomial functions, and satisfies the partition of unity both in real ans Fourier spaces.
  2. $K^G(x)$ is more flat than $K^{JE}(x)$ around $x=0$:
$$\begin{array}{rcl} K^{G}(x) &\approx& 1. - 7.91667 |x|^3 \\\ K^{JE}(x) &\approx& 1. -4.17375 x^2 \end{array}$$
  1. $\widehat{K}^G(u)$ is a little more flat than $\widehat{K}^{JE}(u)$ around $u=0$:
$$\begin{array}{rcl} \widehat{K}^{G}(u) &\approx& 1.-127.168 u^6 \\\ \widehat{K}^{JE}(u) &\approx&1.-188.312 u^6 \end{array}$$
  1. $\widehat{K}^{JE}(u)$ is more flat than $\widehat{K}^G(u)$ around $u=1$ ($j=1$ ghost):
$$\begin{array}{rcl} \widehat{K}^{G}(u) &\approx& -34.2608 (u-1.)^5 \\\ \widehat{K}^{JE}(u) &\approx& 70.4229 (u-1.)^6 \end{array}$$
  1. $\widehat{K}^{JE}(u)$ has higher $j&gt;1$ ghosts than $\widehat{K}^{G}(u)$ has a matter of compensation of the 1st ghost intensity. This is illustrated below by the absolute value of the coeff. of the expension of $\hat{K}(u)$ around $u=j$.
    image

To get a visual inspection of 1st ghost comparison as Fig 1 of Gary's 2014 paper
image
and concerning a Shear exercise as the one of Gary's paper
image

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have written a summary on the comment of the QinticBis class in the interpolant.py file. But I'm not sure of the rendering of the Latex formula.

Copy link
Author

@jecampagne jecampagne Dec 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm trying to recover test_interpolants.py.
Well, I 'm stuck as it is not only the old python syntax which is easy to modify, but the API is the release 1.0 one more or less and I do not really see how to fill the gap with the present 2.5 release. The difficulties (at least for me) is the get_config function that prepare the configuration for the main part of the code. Need some help.

Concerning test_interpolants_parametric.py, I've translated python 2 cPickle into pickle python 3, and made the syntax adaptation. Now, not sure that the output file would readable: I've change "wb" to "w" as python produces an error (TypeError: a bytes-like object is required, not 'str')... At least it is running (quite slow indeed, may be due to lack of parallel computing settings from my poor knowledge). Let us see what will be the result.

Copy link
Author

@jecampagne jecampagne Dec 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've succeeded to run w/o crashes

 python test_interpolants_parametric.py

with use_interpolants = ['quintic','quinticbis',] with x4 and x6 padding only, has it takes too long to get all interpolants, and I guess we wnat to focus on the differences btw both quintic kernels.
I got then two files

interpolant_test_parametric_output_delta.dat
interpolant_test_parametric_output_original.dat

which are full of digits, eg.

0 1 7 4 0.0 0.0 1.0 0.0 0.0 0.0 0.25770524553031665 0.25770524553031665 -3.744121318538518e-08 -0.1704341300636095 -0.1704341300636095 3.1705050529406975e-09 12.274123191833496 12.274123191833496 0.0
1 1 7 4 0.0 0.0 1.0 0.0 0.0 0.0 0.17104476123913895 0.17104476123913895 0.0 -0.08506195174953833 -0.08506195174953833 0.0 24.92190933227539 24.92190933227539 0.0
2 1 7 4 0.0 0.0 1.0 0.0 0.0 0.0 -0.11189117548202691 -0.11189117548202691 6.33555060775004e-08 0.11695983838376801 0.11695983838376801 -2.522577972896567e-08 9.526039123535156 9.526039123535156 2.0022473223946557e-07
3 1 7 4 0.0 0.0 1.0 0.0 0.0 0.0 -0.11641141490902396 -0.11641141490902396 1.1733047446116363e-07 -0.37654191463949516 -0.37654191463949516 1.4869379583171138e-07 8.769744873046875 8.769744873046875 3.2623787700049066e-07
4 1 7 4 0.0 0.0 1.0 0.0 0.0 0.0 0.08082227985345199 0.08082227985345199 -3.4843277755311486e-05 -0.1910075380407282 -0.1910075380407282 2.510564681271199e-05 3.2600162029266357 3.2600162029266357 0.00021194282458352468
5 1 7 4 0.0 0.0 1.0 0.0 0.0 0.0 -0.17088581813656703 -0.17088581813656703 1.633058779870744e-08 0.06538689404139647 0.06538689404139647 -3.560703398797216e-10 15.034000396728516 15.034000396728516 6.343450121324828e-08
6 1 7 4 0.0 0.0 1.0 0.0 0.0 0.0 0.08044154702195001 0.08044154702195001 0.0 0.004146179231359682 0.004146179231359682 0.0 29.20765495300293 29.20765495300293 0.0
7 1 7 4 0.0 0.0 1.0 0.0 0.0 0.0 -0.2006712890480676 -0.2006712890480676 1.743652874863777e-08 0.11759667667616072 0.11759667667616072 -8.867007947332972e-09 14.169123649597168 14.169123649597168 0.0
8 1 7 4 0.0 0.0 1.0 0.0 0.0 0.0 -10.0 -10.0 0.0 -10.0 -10.0 0.0 -10.0 -10.0 -0.0
9 1 7 4 0.0 0.0 1.0 0.0 0.0 0.0 -0.26816091307716905 -0.26816091307716905 7.530807773392656e-05 -0.2307710341157043 -0.2307710341157043 2.6944817052981795e-05 3.7241814136505127 3.7241814136505127 0.00024256820382182243

I've seen then plot_test_interpolants.py which I have tuned blindly

interpolant_titles = ['quintic','quinticbis','default']
interpolant_colors = ['#000000', '#db00db', '#f80000']
padding_titles = ['pad4','pad6']
padding_shapes = ['o', 's']

With the "delta" output file I got many PNG files, here is the full list:

-rw-r--r-- 1 campagne 29K Dec 25 11:14 delta_out_quintic_bg_je.x.delta_g1_as_f_of_expected_g1.change-g1.png
-rw-r--r-- 1 campagne 32K Dec 25 11:14 delta_out_quintic_bg_je.x.delta_g1_as_f_of_expected_minus_intrinsic_g1.change-g1.png
-rw-r--r-- 1 campagne 29K Dec 25 11:14 delta_out_quintic_bg_je.x.delta_g1_as_f_of_applied_g1.change-g1.png
-rw-r--r-- 1 campagne 29K Dec 25 11:14 delta_out_quintic_bg_je.x.delta_g2_as_f_of_expected_g2.change-g1.png
-rw-r--r-- 1 campagne 29K Dec 25 11:14 delta_out_quintic_bg_je.x.delta_g2_as_f_of_applied_g1.change-g1.png
-rw-r--r-- 1 campagne 27K Dec 25 11:14 delta_out_quintic_bg_je.x.delta_frac_size_as_f_of_expected_size.change-g1.png
-rw-r--r-- 1 campagne 22K Dec 25 11:14 delta_out_quintic_bg_je.x.g1_errors.change-g1.png
-rw-r--r-- 1 campagne 24K Dec 25 11:14 delta_out_quintic_bg_je.x.g2_errors.change-g1.png
-rw-r--r-- 1 campagne 27K Dec 25 11:14 delta_out_quintic_bg_je.x.frac_size_errors.change-g1.png
-rw-r--r-- 1 campagne 30K Dec 25 11:14 delta_out_quintic_bg_je.x.delta_g1_as_f_of_expected_g1.change-g2.png
-rw-r--r-- 1 campagne 30K Dec 25 11:14 delta_out_quintic_bg_je.x.delta_g1_as_f_of_applied_g2.change-g2.png
-rw-r--r-- 1 campagne 29K Dec 25 11:14 delta_out_quintic_bg_je.x.delta_g2_as_f_of_expected_g2.change-g2.png
-rw-r--r-- 1 campagne 31K Dec 25 11:14 delta_out_quintic_bg_je.x.delta_g2_as_f_of_expected_minus_intrinsic_g2.change-g2.png
-rw-r--r-- 1 campagne 29K Dec 25 11:14 delta_out_quintic_bg_je.x.delta_g2_as_f_of_applied_g2.change-g2.png
-rw-r--r-- 1 campagne 24K Dec 25 11:14 delta_out_quintic_bg_je.x.delta_frac_size_as_f_of_expected_size.change-g2.png
-rw-r--r-- 1 campagne 22K Dec 25 11:14 delta_out_quintic_bg_je.x.g1_errors.change-g2.png
-rw-r--r-- 1 campagne 25K Dec 25 11:14 delta_out_quintic_bg_je.x.g2_errors.change-g2.png
-rw-r--r-- 1 campagne 28K Dec 25 11:14 delta_out_quintic_bg_je.x.frac_size_errors.change-g2.png
-rw-r--r-- 1 campagne 31K Dec 25 11:14 delta_out_quintic_bg_je.x.delta_g1_as_f_of_expected_g1.change-mag.png
-rw-r--r-- 1 campagne 30K Dec 25 11:14 delta_out_quintic_bg_je.x.delta_g2_as_f_of_expected_g2.change-mag.png
-rw-r--r-- 1 campagne 30K Dec 25 11:14 delta_out_quintic_bg_je.x.delta_frac_size_as_f_of_expected_size.change-mag.png
-rw-r--r-- 1 campagne 29K Dec 25 11:14 delta_out_quintic_bg_je.x.delta_frac_size_as_f_of_applied_magnification.change-mag.png
-rw-r--r-- 1 campagne 23K Dec 25 11:14 delta_out_quintic_bg_je.x.g1_errors.change-mag.png
-rw-r--r-- 1 campagne 25K Dec 25 11:14 delta_out_quintic_bg_je.x.g2_errors.change-mag.png
-rw-r--r-- 1 campagne 27K Dec 25 11:14 delta_out_quintic_bg_je.x.frac_size_errors.change-mag.png
-rw-r--r-- 1 campagne 32K Dec 25 11:14 delta_out_quintic_bg_je.x.delta_g1_as_f_of_expected_g1.change-all.png
-rw-r--r-- 1 campagne 34K Dec 25 11:14 delta_out_quintic_bg_je.x.delta_g1_as_f_of_expected_minus_intrinsic_g1.change-all.png
-rw-r--r-- 1 campagne 31K Dec 25 11:14 delta_out_quintic_bg_je.x.delta_g2_as_f_of_expected_g2.change-all.png
-rw-r--r-- 1 campagne 29K Dec 25 11:14 delta_out_quintic_bg_je.x.delta_frac_size_as_f_of_expected_size.change-all.png
-rw-r--r-- 1 campagne 33K Dec 25 11:14 delta_out_quintic_bg_je.x.delta_frac_size_as_f_of_applied_magnification.change-all.png
-rw-r--r-- 1 campagne 23K Dec 25 11:14 delta_out_quintic_bg_je.x.g1_errors.change-all.png
-rw-r--r-- 1 campagne 26K Dec 25 11:14 delta_out_quintic_bg_je.x.g2_errors.change-all.png
-rw-r--r-- 1 campagne 30K Dec 25 11:14 delta_out_quintic_bg_je.x.frac_size_errors.change-all.png
-rw-r--r-- 1 campagne 31K Dec 25 11:14 delta_out_quintic_bg_je.x.delta_g1_as_f_of_expected_g1.change-shift1.png
-rw-r--r-- 1 campagne 30K Dec 25 11:14 delta_out_quintic_bg_je.x.delta_g2_as_f_of_expected_g2.change-shift1.png
-rw-r--r-- 1 campagne 25K Dec 25 11:14 delta_out_quintic_bg_je.x.delta_frac_size_as_f_of_expected_size.change-shift1.png
-rw-r--r-- 1 campagne 23K Dec 25 11:14 delta_out_quintic_bg_je.x.g1_errors.change-shift1.png
-rw-r--r-- 1 campagne 25K Dec 25 11:14 delta_out_quintic_bg_je.x.g2_errors.change-shift1.png
-rw-r--r-- 1 campagne 28K Dec 25 11:14 delta_out_quintic_bg_je.x.frac_size_errors.change-shift1.png
-rw-r--r-- 1 campagne 31K Dec 25 11:14 delta_out_quintic_bg_je.x.delta_g1_as_f_of_expected_g1.change-shift2.png
-rw-r--r-- 1 campagne 30K Dec 25 11:14 delta_out_quintic_bg_je.x.delta_g2_as_f_of_expected_g2.change-shift2.png
-rw-r--r-- 1 campagne 25K Dec 25 11:14 delta_out_quintic_bg_je.x.delta_frac_size_as_f_of_expected_size.change-shift2.png
-rw-r--r-- 1 campagne 22K Dec 25 11:14 delta_out_quintic_bg_je.x.g1_errors.change-shift2.png
-rw-r--r-- 1 campagne 25K Dec 25 11:14 delta_out_quintic_bg_je.x.g2_errors.change-shift2.png
-rw-r--r-- 1 campagne 28K Dec 25 11:14 delta_out_quintic_bg_je.x.frac_size_errors.change-shift2.png

but get a crash

Traceback (most recent call last):
  File "/sps/lsst/users/campagne/GalSim-r2.5-forked/devel/modules/plot_test_interpolants.py", line 319, in <module>
    plot_interpolants(sys.argv[-2], sys.argv[-1])
  File "/sps/lsst/users/campagne/GalSim-r2.5-forked/devel/modules/plot_test_interpolants.py", line 289, in plot_interpolants
    plt.errorbar(bins[k]+ixoffset*xoffset, mean, yerr=[q1,q3],
  File "/pbs/home/c/campagne/my_sps/miniconda3/envs/galsim/lib/python3.11/site-packages/matplotlib/pyplot.py", line 3027, in errorbar
    return gca().errorbar(
           ^^^^^^^^^^^^^^^
  File "/pbs/home/c/campagne/my_sps/miniconda3/envs/galsim/lib/python3.11/site-packages/matplotlib/__init__.py", line 1465, in inner
    return func(ax, *map(sanitize_sequence, args), **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/pbs/home/c/campagne/my_sps/miniconda3/envs/galsim/lib/python3.11/site-packages/matplotlib/axes/_axes.py", line 3674, in errorbar
    raise ValueError(
ValueError: 'yerr' must not contain negative values

So may be the list is not complete.

Now, the PNG are plots but with legend 'quinticbis' & 'default' only, that is to say w/o any "quintic" marker. So it might be that the settings

interpolant_titles = ['quintic','quinticbis','default']
interpolant_colors = ['#000000', '#db00db', '#f80000']

are not correct.

Finaly, the "original" dat file produces also the same error crash, but 1 PNG less.

So, if someone can remind who has done these codes and how to adapt to the current API, it would be great.

Copy link
Author

@jecampagne jecampagne Dec 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi again, debugging lead that the interpolant indexes are[1, 2, 7] so tweeking a bit

interpolant_titles = ['dummy','quintic','quinticbis'
                          ,'dummy','dummy','dummy','default']

leads removing the yerr crash. I just get

Unable to parse the pattern
/usr/lib64/python2.7/site-packages/matplotlib/axes.py:4602: UserWarning: No labeled objects found. Use label='...' kwarg on individual plots.
  warnings.warn("No labeled objects found. "

Now looking at some PNG seems to get quintic and quinticbis marker but it is quite blind analysis and I not sure to get valuable results. Now the results look very similar.

Which is strange looking at Figure3 like images. May be the g1/g2 estimator is dominated by something which is greater than the difference of the two kernels....

galsim/interpolant.py Outdated Show resolved Hide resolved
double pi3 = M_PI * pi2;
double pi5 = pi2 * pi3;

#ifdef USE_TABLES
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to include a USE_TABLES version. We don't use it. And honestly, we should probably remove all those bits from this file. These were Gary's original implementation, and when I switched over to analytic formulae for a bunch of things, I wasn't sure they were necessarily always faster. But I now think they are, and they're definitely always more accurate. So there's no need to keep this going in a new class here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I remove these USE_TABLES lines.

galsim/interpolant.py Outdated Show resolved Hide resolved
@rmjarvis
Copy link
Member

rmjarvis commented Mar 27, 2024

Jean-Eric, I'm just checking on the status of this PR. There doesn't seem to be any progress since Christmas. At the time, my request was to describe what the pros and cons are of this interpolant relative to the normal Quintic. I see you added a bunch of very mathy differences. But I don't think a typical user would know what to do with those. What I was looking for was in-practice differences in the rendered images when this is used for k_interpolant in InterpolatedImage compared to the regular Quintic. E.g. the prominance of side lobes or the fidelity of the shear or size recovered from hsm.

Do you have any appetite for doing such investigations to give users appropriate guidance? If so, we can keep this PR open. But if not, I'm inclined to close it as unnecessary.

If you do want to keep it open, I'll note that the tests are failing. Also, it should be rebased onto main, not releases/2.5.

@rmjarvis
Copy link
Member

OK, I'm assuming the lack of response means you've abandoned this effort. Feel free to reopen if you want to try to address the above points.

@rmjarvis rmjarvis closed this May 10, 2024
@rmjarvis rmjarvis added this to the No action milestone May 10, 2024
@rmjarvis rmjarvis added numerics Involving details of the numerical algorithms for performing some calculation(s) feature request Request for a new feature in GalSim labels May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Request for a new feature in GalSim numerics Involving details of the numerical algorithms for performing some calculation(s)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants