Skip to content

Additional zero protection in MSD.#4775

Merged
PDoakORNL merged 1 commit into
QMCPACK:developfrom
ye-luo:msd-nan-protection
Oct 16, 2023
Merged

Additional zero protection in MSD.#4775
PDoakORNL merged 1 commit into
QMCPACK:developfrom
ye-luo:msd-nan-protection

Conversation

@ye-luo

@ye-luo ye-luo commented Oct 13, 2023

Copy link
Copy Markdown
Contributor

Proposed changes

preventing NaN by indirectly protecting zero.

What type(s) of changes does this code introduce?

  • Bugfix

Does this introduce a breaking change?

  • No

What systems has this change been tested on?

epyc-server

Checklist

  • Yes. This PR is up to date with current the current state of 'develop'

@PDoakORNL PDoakORNL left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I need some explanation of why this distributed defensive coding is better than clear initialization of curRatio or its mw_res equivalent. Or if I'm just not understanding how a ratio can go to zero during a series monte carlo moves explain...

Comment thread src/QMCWaveFunctions/Fermion/MultiDiracDeterminant.2.cpp
const int det_id = getDetID(iat);
curRatio = Dets[det_id]->getRefDetRatio() * new_psi_ratio_to_new_ref_det_ / psi_ratio_to_ref_det_;
curRatio = Dets[det_id]->getRefDetRatio();
if (curRatio != PsiValueType(0))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't have a good feel for why it would ever be valid for the curRatio to be zero that seems like it would require some per particle ratio to sum to zero or a result of getting burned by the rather unclear initialization of the single walker curRatio state variable.

Is this just temporary defensive coding?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

When the proposed configuration is on the nodal surface. curRatio goes to zero. This move should be rejected eventually by Monte Carlo. When Dets[det_id]->getRefDetRatio() is zero, new_psi_ratio_to_new_ref_det_ the sum of all the dets with respect to the ref one is NaN because it is the sum of all the 1/0. So keeping curRatio zero should be healthy.

PDoakORNL
PDoakORNL previously approved these changes Oct 13, 2023
@PDoakORNL

Copy link
Copy Markdown
Contributor

test this please

@ye-luo

ye-luo commented Oct 13, 2023

Copy link
Copy Markdown
Contributor Author

I still need to fix the compilation once #4772 is merged first.

When the proposed configuration is on the nodal surface.
Reference determinant curRatio goes to zero.
This move should be rejected eventually by Monte Carlo.
However, to make the code proceed, some NaN protection is needed.
When Dets[det_id]->getRefDetRatio() is zero, new_psi_ratio_to_new_ref_det_ the sum of all the dets
with respect to the ref one is NaN because it is the sum of all the 1/0.
So keeping curRatio zero should be healthy.
@ye-luo ye-luo force-pushed the msd-nan-protection branch from 34ed93d to e18a785 Compare October 13, 2023 23:14
@ye-luo

ye-luo commented Oct 13, 2023

Copy link
Copy Markdown
Contributor Author

Test this please

@PDoakORNL PDoakORNL left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Rebase looks fine

@PDoakORNL PDoakORNL merged commit d0d456e into QMCPACK:develop Oct 16, 2023
@ye-luo ye-luo deleted the msd-nan-protection branch October 31, 2023 01:39
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.

2 participants