Form Factor code with data table; CSV.jl dependency removed.

[sakibmatin]

J0 and J2 constants stored in Sunny/src/data/

Early draft. Any feedback on code or data table location with in Sunny?

[kbarros]

Thanks for your pull request. Overall it looks good. I would suggest to remove the CSV.jl dependency because it brings many sub-dependencies, and we can probably do the parsing pretty easily ourselves.

Along these lines (but not directly related to this PR) is that another big offender is the CIF reading package. If we could one day write a custom parser instead, it may help with Sunny load times, although this should be tested. Julia 1.8 actually brings a nice macro to check load times, https://julialang.org/blog/2022/08/julia-1.8-highlights/#package_load_timing Would you be interested to try it with Sunny Sakib?

[sakibmatin]

I agree. It should be straight foward to parse the data tables without CSV.jl, which clearly complicates the dependencies. I'll update the code.

I don't think I have the bandwidth to code up a custom cif parser for Sunny at this point. I'd be happy to try it out down the line.

[kbarros]

Agreed. I was thinking the action item is to run the profiler and measure what things are actually a bottleneck.

Kipton Barros, from phone

On Sat, Aug 20, 2022, 20:05 Sakib Matin @.***> wrote:

I agree. It should be straight foward to parse the data tables without CSV.jl, which clearly complicates the dependencies. I'll update the code.

I don't think I have the bandwidth to code up a custom cif parser for Sunny at this point. I'd be happy to try it out down the line.

— Reply to this email directly, view it on GitHub https://github.com/SunnySuite/Sunny.jl/pull/28#issuecomment-1221450268, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACG3HPP3M5G6RD5CZIXMFDV2GFFZANCNFSM57CPTGFQ . You are receiving this because you commented.Message ID: <SunnySuite/Sunny .@.***>

[kbarros]

This is looking good to me. Last thing I can see is to rename

FormFactor() -> form_factor()

Also, what's the intended usage? Will this be called by end users, or will this be built into the structure factor measurement?

Cc. @ddahlbom

[ddahlbom]

It's great to see this. Experimentalists will want this off the bat.

My first instinct was to suggest this be applied behind the scenes as an option to dynamic_structure_factor, but I'm not opposed to making it publicly available. Perhaps a user would like to apply the form factor after calculating the structure factor so they can see the effect.

One issue: I think the function should be adapted to act on arrays of q values, loading and parsing the text files only once.

After than we can write an apply_form_factor!(sf::StructureFactor, lattice::Lattice, elem::String, lande::Bool=false) function that wraps this up. I'd be happy to do that -- I'm tweaking the structure factor stuff now -- but also happy if Sakib wants to do it.

[kbarros]

Sakib and I spent some time doing refactors today. David, if you're willing, please take over from here and integrate directly into the structure factor code. Sakib fork gives everybody write permissions, apparently.

Note that the is an optional "second order Landé correction" that is opt-in for the user. Not sure how you want to expose the user interface.

[kbarros]

Something doesn't make sense to me in this table:

    g_dict = Dict{String,Float64}(
        "La3"=>0,
        "Ce3"=>6/7,
        "Pr3"=>4/5,
       ...

First, the ions La3 and Ce3 are not listed in the form factor data files. Should they be added? Second, a Landé g-factor of 0 seems to introduce a divide by zero. In that case, should elements like La3 be removed from the dict?

[ddahlbom]

Sure, I'd be happy to integrate it when you guys are happy.

I already have some structure factor revisions in progress on another branch. Maybe it would be easiest if this PR is just merged, so the function and data are available in main. Then I can rebase on top of it and integrate it with the rest of the revisions. Does that make sense, or is another approach better?

[sakibmatin]

I agree. We should remove elements with a Landé g-factor of 0, as it introduces the division by zero error. We could keep the "Ce3" in the g_dict. We may choose to update the J0 and J2 constants later from other data sources if they become available.

[kbarros]

Where did the table of Landé g factors come from (we should cite the source)? If "0" is a meaningful value, then maybe we should leave those entries, and add more checks to the subroutine.

[kbarros]

The docs are unclear about what the input q is. I assume we will eventually want to get form factors for a Vector{Vec3}, i.e. a list of q-vectors? Does this function actually expect a Vector{Float64}, i.e. a list of magnitudes of q vectors?

It seems the definition is given here: https://www.ill.eu/sites/ccsl/ffacts/ffactnode3.html

Here, it says $s = \sin(\theta) / \lambda$. I'm not sure how that connects with our choice $s = q / 2 \pi$, where presumably $q = |\mathbf{q}|$.

Update: I think I get it now. I will update the doc string.

[ddahlbom]

For every point in momentum space we'll have a vector $\mathbf{q}$, which is just the coordinates in terms of the normalized reciprocal lattice vectors. The form factor calculation first involves calculating $\mathbf{q}' = \frac{\mathbf{q}}{4\pi}$ and then works with the scalar $\mathbf{q}' \cdot \mathbf{q}'$. We want to perform this operation for every point in momentum space.

So, ultimately we have to do this calculation on a list of 3-vectors.

(At least, this is what Xioajian instructed me to do when I was doing the FeI2 calculations. The 4\pi versus 2\pi thing might be a convention, i.e. may depend on how the form factor data was calculated.)

[kbarros]

This routine internally applies the $1/4\pi$ factor. I think the input to this routine should be $q = |\mathbf{q}|$, from which we calculate $F(s)$ using $s = q / 2\pi$. From there, we can rescale the structure factor $S(\mathbf{q})$ according to $F(s)$. Does that sound right?

[ddahlbom]

For what it's worth, this is how I calculated the qs in the past. I think Sunny has a built-in reciprocal lattice vector function, so writing the first function was probably unnecessary.

EDIT: Saw your question. Here also is the form factor calculation itself. (I also put the unnecessary ./, but it does emphasize that we're dealing with a vector.)

[kbarros]

BTW, the relevant documentation is on this page: https://www.ill.eu/sites/ccsl/ffacts/ffactnode3.html

If you squint, this definition of $s = \sin \theta / \lambda$ can be interpreted as $s = q / 2 \pi $, where $q$ denotes momentum transfer for a neutron of incoming momentum $2 \pi / \lambda$. Not sure how I'm missing a factor of 2 with this calculation.

[ddahlbom]

There was a factor of 2 I also found mysterious which Xiaojian insisted I put in...

[kbarros]

I just force-pushed a commit which reintroduces the Lande factors with 0, because presumably these have some physical meaning (it might be a Dict we want to reuse in other contexts).

I have also revised the doc string, for future reference.

David and I had a long conservation and I think we agree with the input $q = |\mathbf q|$ should be a list of scalars representing the magnitudes of the $\mathbf q$-vectors. I think we also agree that the normed_qs() subroutine is not needed.