Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Effective masses #21

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Effective masses #21

wants to merge 2 commits into from

Conversation

fzierler
Copy link
Collaborator

@fzierler fzierler commented Jun 7, 2021

Effective mass for Numbers and DataPoints including error propagation. Still needs tests and requires #20.

@fzierler fzierler added the WIP Work in progress label Jun 7, 2021
@briederer
Copy link
Owner

First look seems fine to me.
Have you also tried building it with the Package mentioned in https://inspirehep.net/literature/1837727 instead of DataPoint from #20?

src/spectroscopy.jl Outdated Show resolved Hide resolved
@briederer
Copy link
Owner

Also the mentioned package ADerrors.jl seems to be very finished.
Maybe we could contact the maintainer and join forces?

@fzierler
Copy link
Collaborator Author

fzierler commented Jun 7, 2021

Also the mentioned package ADerrors.jl seems to be very finished.
Maybe we could contact the maintainer and join forces?

I tried to use ADerrors.jl for performing the propagation of uncertainty of the effective masses, but the resulting error was always an order of magnitude smaller than with the methods in this PR. It might just be that I do not fully understand the error reduction techniques in ADerrors.jl (I will first need to read the references provided in that repo), but for now I am hesitant to start relying on that package since I have not fully understood it (yet?).

Maybe we should think of this package (at least for now) as a collection to most naive implementations for performing lattice spectroscopy. Once we have this working we can add more sophisticated ways of doing this. I will definitely continue to study the details of ADerrors.jl and I think at some point we might be able to provide error propagation using ADerrors.jl in addition to the naive implementations.

We definitely should mention this package in the README.

@briederer
Copy link
Owner

I tried to use ADerrors.jl for performing the propagation of uncertainty of the effective masses, but the resulting error was always an order of magnitude smaller than with the methods in this PR. It might just be that I do not fully understand the error reduction techniques in ADerrors.jl (I will first need to read the references provided in that repo), but for now I am hesitant to start relying on that package since I have not fully understood it (yet?).

Okay seems reasonable.

Maybe we should think of this package (at least for now) as a collection to most naive implementations for performing lattice spectroscopy.

By "this package" you mean our package (i.e. LatSpec.jl) am I right?

We definitely should mention this package in the README.

Feel free to add it immediately.

@fzierler
Copy link
Collaborator Author

fzierler commented Jun 8, 2021

By "this package" you mean our package (i.e. LatSpec.jl) am I right?

Yes

Copy link
Owner

@briederer briederer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found a few more things to be resolved.

src/spectroscopy.jl Outdated Show resolved Hide resolved
src/spectroscopy.jl Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

The Docs for this PR are deployed! 🥳
You can find them here.

Please make sure that you have documented your new feature properly:

  • Added documentation (if needed).

for t in 1:T
err1 = _acoshderiv(c[t]/c[mid])*Δc[t]/c[mid]
err2 = _acoshderiv(c[t]/c[mid])*c[t]*Δc[mid]/c[mid]^2
Δm[t] = LinearAlgebra.norm((err1,err2),norm)/abs(mid-t)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is LinearAlgebra. necessary here?
Not used in the other effective_mass_err-function.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay since you called a variable norm in this function it is reasonable to use explicitly LinearAlgebra.norm() here.
However then I suggest to change it in line 16 too.

m = similar(c)
for i in 1:N
r = c[i]/c[mod1(i+1,N)]
m[i] = r > 0 ? abs(log(r)) : NaN
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about the abs() function here.
Personally I prefer to keep the minus sign explicitely. Though I am not sure what is actually the correct way according to the community.

Copy link
Collaborator Author

@fzierler fzierler Jul 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that there is a correct way of doing this, but all books seem to use the version without abs(). Of course this does not matter if you only consider the range up to N_T/2 so I am fine with dropping the abs().

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I also think without abs() is a bit cleaner (and probably minimally faster 😉).
Especially without this you can more easily differ between a sinh()- or cosh()-behaviour wether the interpolators are sink or source ones.

@briederer
Copy link
Owner

I guess this would need an adaption to the new PR in #25.
Should I merge the current state of this PR into #25 and resolve the differences and close this one?
In the end effective masses should be a fundamental feature of LatSpec so it should be fine to have one submit to master where we rework the overall type and add this.

@fzierler
Copy link
Collaborator Author

I guess this would need an adaption to the new PR in #25. Should I merge the current state of this PR into #25 and resolve the differences and close this one? In the end effective masses should be a fundamental feature of LatSpec so it should be fine to have one submit to master where we rework the overall type and add this.

Yes, I think this is a good approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants