search

LupoA / lsdensities

Smeared spectral densities from lattice correlators
GNU General Public License v3.0
3 stars 0 forks source link

Feature/package #41

Closed nickforce989 closed 6 months ago

nickforce989 commented 6 months ago

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 commented 6 months ago

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

nickforce989 commented 6 months ago

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!

LupoA commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

@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 commented 6 months ago

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 commented 6 months ago

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

LupoA commented 6 months ago

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 commented 6 months ago

Again, I might be doing something silly, but

python runExact.py

returns

ModuleNotFoundError: No module named 'lsdensities'

edbennett commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

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

LupoA commented 6 months ago

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 commented 6 months ago

This appears to be the same issue as discussed in https://github.com/pypa/setuptools/issues/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 commented 6 months ago

got it. thanks!!!