Add new noise model for discrete cosine transform (DCT)

[Bai-Chiang]

code required for enabling discrete cosine transform in mlmapmaker simonsobs/sotodlib#1449

[mhasself]

Thanks.

Please add comments to the top of these two functions, to explain their distinct use cases. It is a good idea to point out what args are treated differently by the two, because the body of the code is very similar for the two functions.

[Bai-Chiang]

This commit ffe9e04 changed existing nmat_detvecs_apply interface. if we don't update sotodlib it will break existing code. Is it better just add the new function in commit 5cb1ed9 and keep existing compatibility?

[mhasself]

Ah, that was not clear. Ya, I don't see any reason to break existing code. Please leave original function intact and add the new thing you need, with an informative function name.

[Bai-Chiang]

The difference between nmat_detvecs_apply_no2xbins and nmat_detvecs_apply function

@@ -1,4 +1,4 @@
-void nmat_detvecs_apply(const bp::object & ft, const bp::object & bins, const bp::object & iD, const bp::object & iV, float s, float norm) {
+void nmat_detvecs_apply_no2xbins(const bp::object & ft, const bp::object & bins, const bp::object & iD, const bp::object & iV, float s, float norm) {
     // Should pass in this too
     BufferWrapper<float>               ft_buf  ("ft",   ft,   false, std::vector<int>{-1,-1});
     BufferWrapper<int32_t>             bins_buf("bins", bins, false, std::vector<int>{-1, 2});
@@ -14,8 +14,7 @@
         throw buffer_exception("iD must be C-contiguous along last axis");
     if (iV_buf->strides[2] != iV_buf->itemsize || iV_buf->strides[1] != iV_buf->itemsize*nvec || iV_buf->strides[0] != iV_buf->itemsize*nvec*ndet)
         throw buffer_exception("iV must be C-contiguous along last axis");
-    // Internally we work with a real view of ft, with twice as many elements to compensate
-    //int nmode = 2*nfreq;
+
     float   * ft_   = (float*)   ft_buf->buf;
     int32_t * bins_ = (int32_t*) bins_buf->buf;
     float   * iD_   = (float*)   iD_buf->buf;
@@ -23,8 +22,8 @@
 
     // Ok, actually do the work
     for(int bi = 0; bi < nbin; bi++) {
-        int b1 = min(2*bins_[2*bi+0],nmode-1);
-        int b2 = min(2*bins_[2*bi+1],nmode);
+        int b1 = min(bins_[2*bi+0],nmode-1);
+        int b2 = min(bins_[2*bi+1],nmode);
         int nm = b2-b1;
         float * biD = iD_ + bi*ndet;
         float * biV = iV_ + bi*ndet*nvec;

[mhasself]

Ok ... if there's really only these 2 little differences, can I propose:

• Make this all a single function, with a new argument "dct_binning=False".
• replace the "2" with bin_scale.
• Set bin_scale = 2 if dct_binning is false; bin_scale = 1 if dct_binning is true.

Then you'll just have 1 function, and minimal duplicated code.

I'm open to other switch names instead of "dct_binning".

Sorry for the thrashing here -- just trying to end up with easy future maintenance!

[Bai-Chiang]

Yes, that does make code cleaner. I didn't know C++ could declare default args.

I merged these two functions. But get these error when testing the code. Do you know where I forgot to change in either so3g or sotodlib?

Boost.Python.ArgumentError: Python argument types in
    so3g.nmat_detvecs_apply(numpy.ndarray, numpy.ndarray, numpy.ndarray, numpy.ndarray, float, float)
did not match C++ signature:
    nmat_detvecs_apply(boost::python::api::object, boost::python::api::object, boost::python::api::object, boost::python::api::object, float, float, bool)

[mhasself]

Do you know where I forgot to change in either so3g or sotodlib?

I think boost python might require you to specify the default explicitly; see G3SuperTimestream.cxx where this is done a bunch. Will be something like

bp::arg("dct_binning")=false,

Also please make sure you smoke test the default as well as dct_binning=True!

[Bai-Chiang]

It seems there's not test script testing nmat_detvecs_apply function. The smoke test is to test BLAS is linked in properly, so it just use call a whatever function that use it.

so3g/test/test_blas.py

Lines 8 to 12 in 75a01c0

	def test_smoke(self):
	"""Confirm that BLAS is linked in properly working by calling a
	function that uses it.
	"""

[mhasself]

Thanks!