Change delta extraction interpolations to cubic [bump minor] #1008
Conversation
There was a problem hiding this comment.
I would add some extra changes (see my other comment). I checked that there are no more related interpolations that would need this change so after this minor thing is fixed we should be able to merge.
I noticed that there are also some interpolations done in the corrections (e.g. calibration correction line 80). However, here the grid is much finer so the change should be much smaller. I'm fine with not changing this for now, but maybe you want to check if this would affect your measurements as well.
Finally, I saw that there are other interpolations when computing the correlation function (for example in picca_wick but I also think we can leave them as they are.
| @@ -243,7 +243,7 @@ def _initialize_get_var_lss(self): | |||
| self.get_var_lss = interp1d(self.log_lambda_var_func_grid, | |||
| var_lss, | |||
| fill_value='extrapolate', | |||
| kind='nearest') | |||
| kind='cubic') | |||
There was a problem hiding this comment.
I would also change this in lines 281, 285, 620 and 624. More for consistency that this having an impact on the measurements
There was a problem hiding this comment.
I didn't really understand what the get_num_pixels and get_valid_fit objects store, and why we even need interpolations for them. It sounds to me like they are supposed to be integers, so I thought the nearest option would be fine for them. What do you think?
There was a problem hiding this comment.
"Nearest" sounds like what is aimed here. If you feel like it, you can get rid of these interpolations while you're at it. They should not be interpolations in my opinion, but I don't know how deeply they are embedded into the code.
There was a problem hiding this comment.
They are basically used in the reports in the delta_attributes files, so that we can check problems in the fits. I don't think they are used elsewhere, so yes, if we don't need it when saving these files, we might as well remove the interpolation.
|
Note that some tests were failing. However, instead of reporting useful information, the test code was crashing. I added a commit to fix it (though the test will still not pass) |
|
|
||
| self.get_mean_flux = interp1d(log_lambda, | ||
| mean_flux, | ||
| fill_value='extrapolate', | ||
| kind='nearest') | ||
| kind='cubic') |
There was a problem hiding this comment.
I have a warning here. Depending on the input file and its noise levels, cubic spline can act terribly. I remember the files from rawio analysis being very noisy. Andrei, you created a smooth spline version. Has that become the norm?
There was a problem hiding this comment.
Good point. Yes the new raw-stats files also save a spline version, but I don't think it's the default. The current files in picca (for 2.4A and 3.2A pixelization) are old and don't have that. So I'm gonna look into updating those as well.
|
@iprafols, @alxogm, @corentinravoux, @andreufont, I've decided to turn off the recomputation of var lss by default when running the true continuum analysis. I've found that in about a third of mocks it would completely mess up everything, leading to serious problems with the correlation function (see attached plot). I've tried manually computing it from a few delta files with the same code as in picca, but I don't see any obvious issues with the interpolation there, so not sure what is causing it. If anyone has some strong opinion about this and is in the mood to debug this, let me know.
|
|
True continuum problems when recomputing var lss are now detailed in a separate issue (#1009). I've also added a warning when that option is used. I think this is all we wanted to do with this pull request, so on my side I'm happy to merge. For completeness here is a plot of the 3D auto-correlation from a DESI Y5 mock with the old and new interpolations. The two are almost identical.
|
|
Great, so now we just need to fix the tests. Note that I added a new commit to fix the failed test reports |
|
Hi @andreicuceu , are there any updates on this PR? |
|
The only remaining step is to update the automatic tests. I've already been using this branch to run all my mocks for the last few weeks, and everything looks good. I've just been too busy lately to look at the tests, and it will probably not happen before the end of the DESI meeting either. |
|
I'll try to update them in between the sessions tomorrow or on Wednesday |
iprafols
changed the title
|
@iprafols fixed the tests, merge whenever you're ready! |
This is meant to address #1007. Changing interpolations in delta extraction from nearest to cubic will break the tests, so we have to update those as well. I've left the get_num_pixels and get_valid_fit interpolations in nearest, as I'm not sure those need updating.