-
Notifications
You must be signed in to change notification settings - Fork 19
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
Sampled correlations #104
Sampled correlations #104
Conversation
docs/src/structure-factor.md
Outdated
# Make a `StructureFactor` and calculate an initial sample | ||
sf = DynamicStructureFactor(sys; Δt=0.05, ωmax=10.0, nω=100) | ||
# Make a `SampledCorrelations` and calculate an initial sample | ||
sc = SampledCorrelations(sys; Δt=0.05, ωmax=10.0, nω=100) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This no longer has an initial sample as suggested by the comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Thanks.
docs/src/structure-factor.md
Outdated
@@ -122,7 +115,7 @@ configurations. Information about calculating instantaneous data from a dynamic | |||
structure factor can be found in the following section. | |||
|
|||
The basic usage for the instantaneous case is very similar to the dynamic case, | |||
except one calls `InstantStructureFactor` instead of `DynamicStructureFactor`. | |||
except one calls `InstantStructureFactor` instead of `SampledCorrelations`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unsure what to do about InstantStructureFactor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems weird to me that calling InstantStructureFactor
builds you a SampledCorrelations
(none of those words are the same anymore)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I thought this was just where we left it in the thread yesterday.
I guess we could have kept the whole Instant
vs Dynamic
dichotomy: SampledDynamicCorrelations
and SampledInstantCorrelations
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about SampledCorrelations
and InstantCorrelations
, where "sampling" refers to the dynamical sampling process? I suppose there is the possibility that we might want to sample InstantCorrelations
over equilibrium snapshots. In that case, another option is SampledCorrelations
vs InstantSampledCorrelations
, but it starts to feel long. I think a common use case will be to call InstantCorrelations
on an energy minimizing configuration, so leaving out "sampled" makes sense to me.
Presumably InstantCorrelations
can be looking at correlations beyond just dipole-dipole, so StructureFactor -> Correlations
makes sense to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the primary use cases of InstantCorrelations
right now? Also note the overlap with print_wrapped_intensities
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
Yep, the warnings are not your fault. It's some Documenter issue, has to do with the recently tweaked docs pipeline. I haven't found a way to resolve them, but I think they occur when you link from one docstring to another docstring. |
Gotcha. I think one warning will go away if a docstring is added to |
Per @ddahlbom's comment:
|
e6021b2
to
3a0721f
Compare
docs/src/versions.md
Outdated
`StructureFactor` type renamed `SampledCorrelations`. An appropriate | ||
`SampledCorrelations` is created by calling either `dynamical_correlations` or | ||
`instant_correlations` instead of `DynamicStructureFactor` or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add hyperlinks,
[`SampledCorrelations`](@ref)
and similarly for dynamical_correlations
and instant_correlations
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Added. (Note that the docstring for SampledCorrelations
is basically just a stub and points to the docstrings for dynamical_correlations
and instant_correlations
. But I think it is useful to export SampledCorrelations
, just because so many locations in the docs really want to refer to SampledCorrelations
itself.)
Looks good to me. Thanks David! |
@@ -46,7 +55,7 @@ can be resolved by passing an explicit `offset`. | |||
The function [`remove_periodicity!`](@ref) disables periodicity along specified | |||
dimensions. | |||
|
|||
Rename `StaticStructureFactor` to [`InstantStructureFactor`](@ref). | |||
Rename `StaticStructureFactor` to [`InstantStructureFactor`]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is gone now, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. That line is describing an earlier version, I thought. Just keeping the record while removing the broken hyperlink.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Guess I forgot to remove the square brackets.)
This just changes the name
StructureFactor
toSampledCorrelations
. It also eliminates the automatic generation of an initial sample when creating aSampledCorrelations
, as was discussed some time back.I am getting some new warnings when building the docs, but they seem to look fine when I inspect them.
There's not much here, but many lines are touched. Let me know if you see any problems.