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

Classical intensities as density #272

Merged
merged 12 commits into from
Jun 7, 2024
Merged

Conversation

ddahlbom
Copy link
Member

@ddahlbom ddahlbom commented Jun 6, 2024

This PR

  • Divides the output of intensities_interpolated by sc.Δωso that the returned values are densities. If sc.Δω=NaN (because sc is an instant structure factor) divides by 1 instead.
  • Updates tests correspondingly.
  • Updates colorranges of heatmap plots in examples.
  • Pre-plans additional FFTs for new correlation scheme and eliminates an allocation.

@ddahlbom ddahlbom force-pushed the classical-intensities-as-density branch 2 times, most recently from 5a05f7c to 4617a99 Compare June 6, 2024 01:16
@ddahlbom ddahlbom force-pushed the classical-intensities-as-density branch from 71a30af to f624b94 Compare June 6, 2024 21:49
@ddahlbom ddahlbom requested a review from kbarros June 6, 2024 22:14
)

heatmap!(ax_bottom,1:size(is_binned,1), ωs, is_binned;
colorrange=(0.0,0.05),
colorrange=(0.0, 1.5e-6),
Copy link
Member

Choose a reason for hiding this comment

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

Did we accidentally introduce a Δω factor into binned intensities? These are already integrated quantities, so no Δω should appear, and the colorrange should not need to change.

Copy link
Member Author

@ddahlbom ddahlbom Jun 6, 2024

Choose a reason for hiding this comment

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

This was the figure that was already "broken" (i.e., appeared blank) before anything was added by this PR. In other words, this change was essentially unrelated to the rest of the PR.

@Lazersmoke I don't think any of the binning functions call intensities_interpolated, so they should not be affected by this PR. All the binning tests still pass. But the binned results in the FeI2 tutorial appear appear like they already needed some plotting tweaks before this PR. See binned plot here. The change here just tweaks the plotting limits so that the results show up, as seen here.

Copy link
Member

@kbarros kbarros Jun 7, 2024

Choose a reason for hiding this comment

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

Per comments below, the binning intensity scale may have been broken in #246 (and possibly related #271?), but is now fixed through the commits in #273, which have been merged into this PR branch.

@kbarros
Copy link
Member

kbarros commented Jun 6, 2024

Looks good to me, pending Sam's comments on binned intensities. Thanks David.

@Lazersmoke
Copy link
Contributor

Hi David, thanks for writing this up.

I think the better way to make this change is to remove the 1/sqrt(n all omega) from the time fft normalization factor. The complete end to end calculation in the time direction is then:

  1. For each time offset t, sum up the |t-T| estimates (by fft method), and divide by that number of estimates. This results in a density already! (It's a number that does not go down when the bin size is decreased)
  2. For interpolation, just return the density (no factor)
  3. For binning, calculate and include the factor (this is done at the end of intensities_binned already, which is why there's a bug happening in the docs)

@Lazersmoke
Copy link
Contributor

(per the discussion around #246 , the overall color scale should have not changed during that, but it turns out it actually did because the aforementioned inclusion of the factor in step 3 happened when that was merged)

@Lazersmoke
Copy link
Contributor

See #273

@kbarros kbarros merged commit b169d68 into main Jun 7, 2024
4 checks passed
@kbarros kbarros deleted the classical-intensities-as-density branch June 7, 2024 22:12
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