Feature/package

[nickforce989]

I have implemented the systematics evaluation, Cauchy kernel and rho fitting procedure starting from the code in 'feature/GP'. This could a good start where one can do the refactoring, as it implements both HLT and Bayesian processes and perform fits and uses two kernels. Also a starting point for the package structure have been created.

Please, note that the fitting procedure is still maybe not the most user friendly, as I kept printRhoSamples.py, that Alessandro wrote, to print the bootstrap samples that then will be used to perform fits.

Also, I would suggest to check if there are modifications needed in printRhoSamples.py for the Bayesian processes.

The code 'runInverseProblem.py' runs and I have been able to perform a fit from that output.

[LupoA]

great work. asking out of ignorance: are the pycache files there by accident or are they needed for packaging? :)

[nickforce989]

great work. asking out of ignorance: are the pycache files there by accident or are they needed for packaging? :)

My bad, I did not notice the pycache files left. I have removed them. Thanks for noticing!

[edbennett]

Thanks Niccolò, this is good progress.

Something to bear in mind is that it is easier to review pull requests that are relatively focused—introducing new features and also restructuring the code at the same time makes it more difficult to spot potential issues.

Remember also that if you push to the branch from which you made your pull request, then it will add the commits to the PR (which if they are unrelated will further increase its scope).

Please take a look at the specific changes requested in the below. Do push the changes to the same branch, because in this case we do want them to become part of this PR. Any questions, please ask!

[LupoA]

Something to bear in mind is that it is easier to review pull requests that are relatively focused—introducing new features and also restructuring the code at the same time makes it more difficult to spot potential issues.

I should probably merge my other PR, since it addressed some of the open issues already

[LupoA]

@nickforce98 are we keeping the normalisation for the gaussian in [0,inf]?

that's fine by me if it's needed for the code release (so you dont have to re-do fits), but I would get rid of it right after. It's quite error prone because the kernel is no longer symmetric in the arguments and I can't vouch the rest of the code is aware of this

edit:
To be safe, we could use runExact.py with the various kernels, and make sure that without error on the correlators we can reproduce the true solution

[nickforce989]

@LupoA, I introduced again the possibility of a half normalised gaussian to be consistent between the paper it will come out and the code release. At least in the version I release on Zenodo I would keep that. Then, it's fine to me to remove it.

The code has the options in the parser: 'FULLNORMGAUSS' (default), 'HALFNORMGAUSS', and 'CAUCHY'.

Thanks also for the suggestion.

[LupoA]

very good, let's indeed keep it for the release.

In order to use runExact.py, you should modify (locally on your machine) line 105 where you should put your kernel of choice. Then in line 152 you should tell the code to compute the coefficients with the same kernel. It should be a painless check, but let me know if I can assist!

[edbennett]

Since there will need to be a code release that wraps this library, could the normalisation be done there and this library kept "pure"? (Can discuss in more detail if my meaning isn't clear here.)

[edbennett]

you should modify (locally on your machine) line 105 where you should put your kernel of choice. Then in line 152 you should tell the code to compute the coefficients with the same kernel

You have to modify the installed library in place? This sounds like a design issue that is out of scope for addressing in this PR but is worth trying to improve.

@LupoA am I reading you correctly? If so then we can open an issue to track this.

[LupoA]

not the library, runExact.py is supposed to be an example file or rather a template for a specific application (performing the reconstruction on synthetic, error-free data). The exact smeared spectral density will depend on the specific kernel.

It is of course possible to input the kernel and let runExact work on any of them, by adding a couple of if statement. If we want this, it should be part of the "additional kernel implementation" effort that is already inside this PR (see commit 'adding cauchy kernel')

[edbennett]

Ahh, I see. In that case the version of runExact.py or similar that Niccolò uses in our analysis should be in the code release for the paper, not the more general library.

[LupoA]

I am quite happy with what @nickforce98 has done having it in the example folder! notice that this specific file is not used in your analysis since you use real data, it is however an important file for checking the implementation is correct

[LupoA]

I've lost track of changes... are we happy with the status or is some more work missing? once we are done I can resolve the conflicts and merge

[nickforce989]

@LupoA I think I have fixed all the problems. Also the test runExact.py is working with CAUCHY.

I am not quite happy with the speed of the high precision numerical integration performed to compute f_t and A_0 in the Cauchy case.

Anyways, I thought for long, and made many attempts (also parallelization or similar) to speed it up, and I still did not find a solution.

I will keep thinking if there's anything else I can do to speed it up, but I should have responded to all the points of the review of this PR.

[edbennett]

I am not quite happy with the speed of the high precision numerical integration performed to compute f_t and A_0 in the Cauchy case.

Perhaps open a separate issue to track that at this point.

I will keep thinking if there's anything else I can do to speed it up, but I should have responded to all the points of the review of this PR.

Great!

[LupoA]

Sweet! I'll try resolving the conflicts and then merge

[LupoA]

This might be just me not being used to deal with python packages, but:

Upon manual installation (git clone, cd directory, pip install . ) I obtain the following

Successfully built UNKNOWN
Installing collected packages: UNKNOWN
Successfully installed UNKNOWN-0.0.0

which is not really a problem, but do you know why it shows up as unknown?

[LupoA]

Again, I might be doing something silly, but

python runExact.py

returns

ModuleNotFoundError: No module named 'lsdensities'

[edbennett]

Upon manual installation (git clone, cd directory, pip install . ) I obtain the following

Until the PR is merged, clone is not sufficient—you also need to git checkout feature/package.

[LupoA]

Until the PR is merged, clone is not sufficient—you also need to git checkout feature/package.

Sorry I did not say but I did to that,

Your branch is up-to-date with 'origin/feature/package'

[edbennett]

Just to document, I had a quick Zoom with Alessandro; it's not clear what was causing his issue, but creating a new virtual environment and retrying worked.

@nickforce989 Take a look at dd046f3; it's an important part of creating a pyproject.toml that I missed when reviewing.

[nickforce989]

Ok, thanks for making me notice! and apologies to @LupoA if I made you lose some time.

[LupoA]

Before merging: I am a bit disturbed by the fact that I can only perform the installation in a new virtual environment. The problem I had, persists if I run outside of one. @edbennett any ideas?

[edbennett]

This appears to be the same issue as discussed in pypa/setuptools#3269 - Debian does weird things to the built-in Python (as you saw earlier when we had to apt install things that are usually integral parts of Python), and something about that process has broken things so Python picks up the wrong version of setuptools, which is unable to parse your pyproject.toml.

The solution is to use virtual environments and/or to avoid using the OS-provided Python. (This is considered good practice anyway.)

[LupoA]

got it. thanks!!!