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

Improve DISF+DCSF summation #619

Open
wants to merge 8 commits into
base: protos
Choose a base branch
from

Conversation

MBartkowiakSTFC
Copy link
Collaborator

Description of work
Neutron total dynamic structure factor analysis does not test if the input datasets (DCSF and DISF) were produced with the correct weights applied. As a result, the datasets frequently end up having the weights applied twice.

Fixes

  • Re-enabled the NDTSF analysis
  • Check what weights were used to produce the input datasets. If "equal" apply weights, if "b_coherent" and "b_incoherent2" don't apply weights, reject other inputs.

To test
Try reproducing a known result of dcsf/disf calculation and add the results to each other using the neutron total dynamic structure factor analysis.

@ChiCheng45
Copy link
Collaborator

I found some inconsistency when different equal or b_inc2/b_coh weighs are used. Here are some examples with the Ar36 trajectory.

DCSF (equal) / DISF (equal)
image

DCSF (b_coh) / DISF (b_inc2)
image

DCSF (equal) / DISF (b_inc2)
image

DCSF (b_coh) / DISF (equal)
image

@gonzalezma
Copy link
Collaborator

I am not sure how this is intended to work. Originally, the NTDSF analysis was introduced to compute a signal as similar as possible to the measured one, i.e. something similar to Sum_ij b_ib_jsqrt(c_ic_j)Scoh_ij + Sum_i b_ic_iSinc_i, with output units in barn/sterad (or fm^2/sterad). By providing also the different contributions, it was then possible to determine the relative importance of the coherent and incoherent contributions, etc. As the DCSF and DISF are normalized, one needs to be careful when summing their results to give the NTDSF. I agree that it is possible, but my advice would be to use the partials of both analysis (so it does not matter the weighting scheme used in their calculation) and recalculate all the needed sums.

As for the issue with the Argon simulation, if the element used in the analysis is Ar36, its b_incoherent is null, so I wonder if this could not affect the tests that you are trying to do.

@MBartkowiakSTFC
Copy link
Collaborator Author

@gonzalezma Let's start the discussion of this PR with this paper https://journals.aps.org/pra/abstract/10.1103/PhysRevA.6.1107 where the coherent and incoherent contributions to the total S(q,w) are described for a mixture of gases (but mainly argon). The S_inc shows the strongest signal at 10 nm^-1 and the S_coh at 20 nm^-1, the respective intensities given as 2.6 and 2.4 meV^-1. So, the coherent and incoherent contributions to S(q,w) are expected to be of comparable intensity.
Using the same scattering lengths as those in the paper, and a trajectory of argon, I can calculate the DCSF and DISF in MDANSE. The Q dependence of the signal at 0 energy transfer is shown here:
Sqw_vs_Q_separate
The maxima of the two curves are still comparable.

If I use the existing main branch to combine the DCSF and DISF, the result looks like this:
Sqw_vs_Q_protos_merged
The scale of the signal is different, and the incoherent contribution is gone.

The code proposed in this PR takes the same DCSF and DISF, and produces the following result:
Sqw_vs_Q_newbranch_merged

It appears to me that the result produced by the main branch is not correct. While it is possible that the calculation implemented in this PR is not optimal, it still seems like an improvement over the previous version. What is your opinion? What result would you expect from the NDTSF analysis in this situation?

@MBartkowiakSTFC
Copy link
Collaborator Author

@ChiCheng45 I was getting a similar result at some point, but then I noticed that I was using an artificial element and the value of 'equal' in the table was set to 0 for this one. All the other elements have 'equal' set to 1. I think we should have 'equal' always evaluate as 1, and not take it from the table.

Here is my comparison of the b_coh/b_incoh2 case and the equal/equal case. The results are still different, but this could also be due to the different q vectors, insufficient trajectory length, system size, etc. Blue curve has DCSF and DISF calculated using 'equal' weights and then summed up. Orange used b_coh, b_incoh2 for DCSF and DISF.
ndtsf_from_equal

@gonzalezma
Copy link
Collaborator

I don't know what it is implemented in the main branch, so I would need to look at the code. In MDANSE 1.5 this was an analysis on its own, while now I think that it is a "secondary" analysis that uses as input the output of the DISF and DCSF analysis that should be run before. Correct me if I am wrong.

If this is the case, I don't have anything against this approach, but I don't understand the use of the weighting. My comment was to indicate that it should not matter which weighting is used in calculating DISF and DCSF, if one uses the partials and not the total DISF and total DCSF.

As for your example, my answer to what result I would expect for the NTDSF is "it depends". E.g. if I assume that the sample is Ar36, the incoherent cross section is null, so the old result where NTDSF = DCSF, with no incoherent contribution seems right. But if you assume that you have natural Ar, then the incoherent contribution should be visible. In fact, I don't think that a monoatomic system such as Ar is the best one to test this. I would suggest using water and testing the total S(Q,w) and their corresponding coh/inc contributions for H2O and D2O.

Copy link
Collaborator

@ChiCheng45 ChiCheng45 left a comment

Choose a reason for hiding this comment

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

I've tested with the latest version and as far as I can tell the results are consistent now when you use equal or scattering length weights or a mixture of the two.
image

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