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

Round beta / delta_tau to get number of slices in DQMC #47

Merged
merged 6 commits into from
Nov 22, 2019

Conversation

MasonProtter
Copy link
Contributor

@MasonProtter MasonProtter commented Nov 13, 2019

I'm working on making a DQMC version of https://github.com/crstnbr/MonteCarlo.jl/blob/master/example/Ising2D.jl. Currently, the documented interface for DQMC says that I specify beta and delta_tau and then it calculates slices from their quotient and slices must be an integer. It's annoying that one has to fine tune delta_tau so that beta / delta_tau is an integer. I propose that this should just be rounded, especially because due to floating point error, choosing delta_tau = beta / 500 does not always mean beta / delta_tau can be converted to an integer.

@carstenbauer
Copy link
Owner

Thanks for bringing this up. This should be discussed and handled in one way or another. In my private dqmc code I can either specify beta or the number of slices, to avoid rounding error situations.

@carstenbauer
Copy link
Owner

I'm wondering whether we should at least produce a warning when rounding. After all round does more than just correcting floating point numerical errors.

Example: beta/delta_tau = 3.321/0.1. Arguably not very realistic but technically allowed. It is not clear to me what the users wants in this case. An error/warning would probably be appropriate. Just setting slices to 33 with no warning/error could potentially be misleading.

Maybe we should even look at the difference beta/delta_tau - round(beta/delta_tau) and check that it is "floating point error small". If yes, round, otherwise, warn/error.

Any thoughts?

@MasonProtter
Copy link
Contributor Author

I'm wondering whether we should at least produce a warning when rounding. After all round does more than just correcting floating point numerical errors.

Yeah, this branch is actually just a local change I made so that a script I'm testing could run. There's definitely a lot more we can do to make it clear to a user what is going on.

Putting your above comments together, it seems that a nice solution would be to have three separate methods concerning beta, delta_tau and slices.

  • User specifies beta and slices. We then compute delta_tau.
  • User specifies delta_tau and slices. We then compute beta.
  • User specifies beta and delta_tau. We then compute slices. With this method, we could do something like @assert beta/delta_tau ≈ round(beta/delta_tau), or we could emit a warning, or we could ask that the user also provide their own rounding function. That way they can choose between say floor, ceil and round or make a function like myround(x) = x ≈ round(x) ? round(x) : error(" ... ")

@codecov
Copy link

codecov bot commented Nov 22, 2019

Codecov Report

Merging #47 into master will increase coverage by <.01%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #47      +/-   ##
==========================================
+ Coverage   74.61%   74.61%   +<.01%     
==========================================
  Files          25       25              
  Lines         969      981      +12     
==========================================
+ Hits          723      732       +9     
- Misses        246      249       +3
Impacted Files Coverage Δ
src/flavors/DQMC/DQMC.jl 73.55% <75%> (+0.15%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b7144bc...8069c90. Read the comment docs.

@MasonProtter
Copy link
Contributor Author

MasonProtter commented Nov 22, 2019

Okay, so it took some really ugly conditionals and whatnot since we can't dispatch on keyword arguments, but I got everything working. The interface I went with for now is

  • User specifies beta and slices. We then compute delta_tau.
  • User specifies delta_tau and slices. We then compute beta.
  • User specifies beta and delta_tau. We then compute slices with round(beta/delta_tau), and emit a warning if !(round(beta/delta_tau) !≈ beta/delta_tau).

Thoughts / comments?

src/flavors/DQMC/DQMC.jl Outdated Show resolved Hide resolved
@carstenbauer
Copy link
Owner

Looks good to me!

@MasonProtter
Copy link
Contributor Author

Not sure why the Travis CI isn't running.

src/flavors/DQMC/DQMC.jl Outdated Show resolved Hide resolved
@carstenbauer
Copy link
Owner

carstenbauer commented Nov 22, 2019

Gazillions of github CI jobs :)

(They will probably fail)

@carstenbauer
Copy link
Owner

Can you fix the tests? It would also be good to have a few tests for the different keyword options.

@MasonProtter
Copy link
Contributor Author

MasonProtter commented Nov 22, 2019

These tests pass locally on my machine (2017 MacBook Pro i5 on MacOS Catalina). Not sure what's wrong. Possibly related to BLAS / LAPACK non-determinism.

@carstenbauer
Copy link
Owner

Yeah, probably something like that. This exact test has also errored for one of the master test runs and magically worked again in the following run. Can you open a separate issue so that we don't forget to fix this?

I'll ignore the test failure here and merge.

@carstenbauer carstenbauer merged commit e6a7f1e into carstenbauer:master Nov 22, 2019
@carstenbauer
Copy link
Owner

I opened an issue myself: #50

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

Successfully merging this pull request may close these issues.

3 participants