kstoreyf / suave Goto Github PK

i have changed the name of this repo to suave to reflect that fact that this is a new estimator with new features, though it is still a fork of the Corrfunc project so i can keep it in sync with upstream changes. i have also created a pip distribution under the name suave.

the issue is, the python subdirectory is still called Corrfunc, so the package must be imported with 'import Corrfunc'. i think there are a few options (cred to @dfm):

1. change all instances of Corrfunc in the repo to suave.
   • could then do import suave
   • would be annoying to keep repo in sync with the upstream
2. keep distribution name and repo name as suave, keep python directory as Corrfunc
   • would then have to do import Corrfunc, and warn users about this
   • users could not keep out-of-the-box Corrfunc separate from suave (should be one-way-compatible such that all regular Corrfunc functionality works the same when called from suave, but would be nice to have ability for them to be separate)
3. something fancy with the python directory named suave containing a subdirectory named Corrfunc, and changing the imports within the code to relative imports?
   • same issues as 1.

@manodeep, do you have any other ideas, or opinions on which of these would be best? thank you!

Congrats again on getting the paper submitted! :)

I took a look at the new code and had the following comments. Hope these are useful and understandable

1. These lines might be faster if re-written as:

    for(int p=0; p<projdata->nsbins;p++){
        u[p] = (sqr_s >= projdata->supp_sqr[p] && sqr_s < projdata->supp_sqr[p+1]) ? ONE:ZERO;
    }

2. Regarding this comment in that file, the compile failure is because of padding bytes being inserted by the compiler to allow correct alignment. The C standard requires that memory addresses for variables must be divisible by the width of the variable -- i.e., 4-byte/32-bit integers must begin on memory addresses divisible by 4, 8-byte/64-bit integers must be allocated on memory addresses divisible by 8 - this is referred to as "alignment". structs and pointers are all 8-byte aligned (for practical purposes). If you look at the definition of the struct extra_options -

struct extra_options
{
    // Two possible weight_structs (at most we will have two loaded sets of particles)
    weight_struct weights0;
    weight_struct weights1;
    weight_method_t weight_method; // the function that will get called to give the weight of a particle pair
    proj_method_t proj_method;
    int nprojbins;
    char *projfn;
    uint8_t reserved[EXTRA_OPTIONS_HEADER_SIZE - 2*sizeof(weight_struct) - sizeof(weight_method_t)
                                              - sizeof(proj_method_t) - sizeof(int) - sizeof(char *)];
};

Since extra_options is a struct, the memory address must be divisible by 8. The first two elements are 8 bytes aligned (they are 88 bytes each), the weight_method_t and proj_method_t are both enums - i.e., 32-bit int - which means they are 4 bytes each. But because there are two of them, the end of proj_method is still divisible by 8. But then int nprojbins appears, which means address at the end of nprojbins is only divisible by 4, but char * projfn is a pointer and needs to have a memory address divisible by 8. Therefore, the compiler inserts 4 padding bytes between nprojbins and projfn. You can see this as a warning if you enabled -Wpadded (or -Wpadding, can't remember). You can fix the error by altering nprojbins to int64_t instead of int, and changing the calculation (sizeof(int) -> sizeof(int64_t)) in reserved. Afterwards, you should also be able to reset the total size of EXTRA_OPTIONS back to 1024 bytes.

3. You should also be able to condense these lines to

                const DOUBLE fac = need_weightavg ? pairweight:ONE;
                for(int p1=0;p1<nprojbins;p1++){
                    projpairs[p1] += u[p1]*fac;
                }
                if (need_tensor) { 
                    for(int p1=0;p1<nprojbins;p1++){
                        for(int p2=0;p2<nprojbins;p2++){
                            projpairs_tensor[ p1*nprojbins + p2] += u[p1] * u[p2] * fac;
                        }
                    }
                }
                

But that might be fetch the data twice - so check that the runtime is not affected dramatically. If the runtime changes by a lot, then your original implementation would be better (but replace the if(need_weightavg) with the multiplication by fac as I have done here).

4. Similarly, these lines can be condensed to:

    if(need_proj) {
      //nsbin is number of edges, want number of bins
      int nsbins = nsbin-1;
      projdata->nsbins = nsbins;
      projdata->supp = supp;
      projdata->supp_sqr = supp_sqr;    
      for(int i=0; i<nprojbins; i++) {
        projpairs[i] = ZERO;
      }
      if(need_tensor) {
        for(int i=0; i<nprojbins*nprojbins; i++) {
          projpairs_tensor[i] = ZERO;
        }
    }

Overall, nothing jumps out as obviously inefficient to me, but a profiling will essential to understand where the hotspots are. To improve performance, the next step would be to add in vectorised kernels for each of the filters

(P.S. I edited code on this GitHub issue - quite likely to contain translation/implementation bug)

General information

• versions: smoothcorrfunc 0.0.0, Corrfunc: 2.3.2
• platform: linux
• installation method (pip/source/other?): source

Issue description

I am trying to make my package installable via pip. I have edited the setup.py and common.mk files to make a new distribution name and version number. When I upload the distribution to testpypi, it seems fine, but then when I try to install on another system, it hits an error because of the PYTHON variable in common.mk.

I figured out that the setup.py file checks that the python executable that ran the setup script is the same as the one in common.mk, and if it's not, it updates the common.mk one to the python path used. This means that my miniconda path ends up hardcoded, so when I try to install on another system it fails.

Actual behavior

This is the line that updates the PYTHON variable and causes the issue:
python setup.py sdist
where python points to my miniconda python installation, /home/users/ksf293/miniconda3/bin/python3.

Then I do
python -m twine upload --repository testpypi --skip-existing dist/*

The package is successfully uploaded to testpypi here:
https://test.pypi.org/project/smoothcorrfunc/0.0.0
and can be installed with:
pip install -i https://test.pypi.org/simple/ smoothcorrfunc==0.0.0

When I try this on a different system, I get the error:
RuntimeError: command = /home/users/ksf293/miniconda3/bin/python3 -c 'from __future__ import print_function; import sys; print(sys.executable)' failed with stdout = b'' stderr = b'/bin/sh: /home/users/ksf293/miniconda3/bin/python3: No such file or directory\n' status 127

Expected behavior

For now I edited setup.py to not update common.mk, and just set PYTHON:=python in common.mk.

I did this for an updated pip distribution, so doing
pip install -i https://test.pypi.org/simple/ smoothcorrfunc==0.0.1
installs with no errors.

Is there a better solution?

proj_type = 'generalr'
kwargs = {'order': 0} # 1: linear spline
projfn = 'quadratic_spline.dat'
nprojbins = int(nbins/1)
spline.write_bases(rmin, rmax, nprojbins, projfn, ncont=1000, **kwargs)

returns

No order given, defaulting to 1 (linear)