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

Incorrect value example of _atom_site_moment_fourier_param.cos_symmform #67

Closed
vaitkus opened this issue Jan 22, 2024 · 8 comments · Fixed by #68
Closed

Incorrect value example of _atom_site_moment_fourier_param.cos_symmform #67

vaitkus opened this issue Jan 22, 2024 · 8 comments · Fixed by #68

Comments

@vaitkus
Copy link
Collaborator

vaitkus commented Jan 22, 2024

The _description_example.case attribute is used in two slightly different ways:

  • In data item definitions it provides example values.
  • In category definitions it provides excerpts from CIF files that demonstrate how items from the given category may be used.

Currently, the definition of the _atom_site_moment_fourier_param.cos_symmform data item incorrectly provides a CIF excerpt:

    loop_
    _cell_wave_vector_seq_id
    _cell_wave_vector_x
# ...
    _atom_site_moment_Fourier_param.sin
    _atom_site_moment_Fourier_param.cos_symmform
    _atom_site_moment_Fourier_param.sin_symmform
    1  Fe_1 1 x  0.00000  0.84852  0            mxs1
    2  Fe_1 1 y  0.00000  0.42426  0            0.50000*mxs1
    3  Fe_1 1 z  0.00000  0.00000  0            0
    4  Fe_1 2 x  0.00000 -0.42426  0           -0.50000*mxs1
    5  Fe_1 2 y  0.00000 -0.84852  0           -mxs1
# ...

It would be best to replace this example with one or two examples of cos symmform values and, if possible, human readable explanations, e.g. as is done with the _atom_site_moment.symmform data item:

    loop_
      _description_example.case
      _description_example.detail
         mx,my,mz
;
         no symmetry restrictions
;
         mx,-mx,0
;
         y component equal and opposite to x component
          with z component zero
;

Alternatively, we could remove this example, since a very similar example with an CIF excerpt is already provided in the ATOM_SITE_MOMENT_FOURIER_PARAM category example.

@brantonc, could you please provide a few symmform value examples and their human-readable descriptions? (Or is the ATOM_SITE_MOMENT_FOURIER_PARAM category example alone is already sufficient?)

@brantonc
Copy link
Collaborator

brantonc commented Jan 29, 2024 via email

@vaitkus
Copy link
Collaborator Author

vaitkus commented Jan 30, 2024

Firstly, let me apologise if my original issue report looks malformed -- I am using the GitHub web interface and it renders great there, however, I am not sure if all the visual formatting translates well in an email.

The magCIF 0.9.8 on the IUCr website that you linked indeed does not have this problem, it is only present in this repository. Specifically, the updated definition now contains the _description_example.case attribute with a quite unusual value. One would expect examples to be simple value-explanation pairs of a similar form to those given for the _atom_site_moment.symmform item (see "Example" section in https://www.iucr.org/__data/iucr/cifdic_html/3/MAGNETIC_CIF/Iatom_site_moment.symmform.html), but instead a category-like example with multiple data names and values is provided (e.g. similar to the one in https://www.iucr.org/__data/iucr/cifdic_html/3/MAGNETIC_CIF/CATOM_SITE_MOMENT_FOURIER_PARAM.html).

So if you would provide several _atom_site_moment_fourier_param.cos_symmform examples, I could replace the current incorrect example (see

magnetic_dic/cif_mag.dic

Lines 1295 to 1332 in 9d05150

_description_example.case
;
loop_
_cell_wave_vector_seq_id
_cell_wave_vector_x
_cell_wave_vector_y
_cell_wave_vector_z
1 0.30000 0.30000 0.00000
2 -0.60000 0.30000 0.00000
loop_
_atom_site_Fourier_wave_vector_seq_id
_atom_site_Fourier_wave_vector_x
_atom_site_Fourier_wave_vector_y
_atom_site_Fourier_wave_vector_z
_atom_site_Fourier_wave_vector_q1_coeff
_atom_site_Fourier_wave_vector_q2_coeff
1 -0.30000 0.60000 0.00000 1 1
2 -0.60000 0.30000 0.00000 0 1
3 -0.30000 -0.30000 0.00000 -1 0
loop_
_atom_site_moment_Fourier_id
_atom_site_moment_Fourier_atom_site_label
_atom_site_moment_Fourier_wave_vector_seq_id
_atom_site_moment_Fourier_axis
_atom_site_moment_Fourier_param.cos
_atom_site_moment_Fourier_param.sin
_atom_site_moment_Fourier_param.cos_symmform
_atom_site_moment_Fourier_param.sin_symmform
1 Fe_1 1 x 0.00000 0.84852 0 mxs1
2 Fe_1 1 y 0.00000 0.42426 0 0.50000*mxs1
3 Fe_1 1 z 0.00000 0.00000 0 0
4 Fe_1 2 x 0.00000 -0.42426 0 -0.50000*mxs1
5 Fe_1 2 y 0.00000 -0.84852 0 -mxs1
6 Fe_1 2 z 0.00000 0.00000 0 0
7 Fe_1 3 x -0.42426 0.00000 -0.50000*mxs1 0
8 Fe_1 3 y 0.42426 0.00000 0.50000*mxs1 0
9 Fe_1 3 z 0.00000 0.00000 0 0
;
).

@brantonc
Copy link
Collaborator

brantonc commented Feb 1, 2024 via email

vaitkus added a commit to vaitkus/magnetic_dic that referenced this issue Feb 5, 2024
…FS#67.

Specifically, examples were added for the
`_atom_site_moment_Fourier_param.modulus_symmform`,
`_atom_site_moment_Fourier_param.phase_symmform` and
`_atom_site_moment_Fourier_param_sin_symmform` data items.
vaitkus added a commit to vaitkus/magnetic_dic that referenced this issue Feb 5, 2024
The previous example was replaced by examples provided in issue COMCIFS#67.
The previous value was merge with the example of the
ATOM_SITE_MOMENT_FOURIER_PARAM category. The category example was only slightly
modified by replacing the dotless data names with their dotted counterparts
and correcting several symmform values that lacked a trailing modulation
number.
@vaitkus
Copy link
Collaborator Author

vaitkus commented Feb 5, 2024

@brantonc , thank you, these are very useful examples!

I created a draft PR (#68) that adds these examples, however, I would like to clarify a few things before merging:

  1. In you earlier email you provided the following description:
Symmetry constraints require the cosine part of a given magnetic vector component of a given propagation vector to either be fixed (e.g. at zero) or else be proportional to one of the independent modulation parameters.  The value of this item indicates the independent modulation parameter and proportionality constant.

Would you like for this description to be included alongside the current definition of symmform items?

  1. The _atom_site_moment_Fourier_param.cos_symmform description provides a detailed description of how the allowed symmform values should be formed. This definition is later on referenced by other symmform items. However, I noticed that some of the examples provided for the _atom_site_moment_Fourier_param.phase_symmform data item do not fully conform to this description. Specifically, value -90 does not match since it is a fixed value other than zero and 180-myp3 does not match since it contains a leading numeric value from which the symbol is subtracted. Maybe _atom_site_moment_Fourier_param.cos_symmform should get a separate fully written out symmform description which accounts for these numeric values?

  2. The _atom_site_moment_Fourier_param.phase_symmform contains the following example:

-90   “Equal to 90 degrees.”

Shouldn't the description read "Equal to -90 degrees"? Or is the sign not important in this case?

  1. The _atom_site_moment_fourier_param.cos_symmform provides a detailed description of the symmform values. The 4th component is described as:
(4) The 4th character is an integer that indicates the modulation vector.

Is the integer value assigned to the modulation vector following some well established rules or does the integer refer to a numeric ID that can be found in the same CIF file (e.g. as _cell_wave_vector.seq_id value or similar)?

  1. Just a note that the PR in question also updates the ATOM_SITE_MOMENT_FOURIER_PARAM category example based on the _atom_site_moment_fourier_param.cos_symmform data item example which is now removed from that item. Specifically, the example contained symmform values without the trailing numeric value mxs instead of mxs1. See:

https://www.iucr.org/__data/iucr/cifdic_html/3/MAGNETIC_CIF/CATOM_SITE_MOMENT_FOURIER_PARAM.html

@brantonc
Copy link
Collaborator

brantonc commented Feb 6, 2024 via email

@brantonc
Copy link
Collaborator

brantonc commented Feb 6, 2024 via email

vaitkus added a commit to vaitkus/magnetic_dic that referenced this issue Feb 6, 2024
…OMCIFS#67.

The symmform symbolic expression description part was copied from the
definition of `_atom_site_moment_Fourier_param.cos_symmform` and adapted to
better reflect the syntax of each individual item.
vaitkus added a commit to vaitkus/magnetic_dic that referenced this issue Feb 6, 2024
As specified in
COMCIFS#67 (comment),
the 4th character is a numeric code as specified in the
_atom_site_moment_Fourier.wave_vector_seq_id data item.
@vaitkus
Copy link
Collaborator Author

vaitkus commented Feb 6, 2024

Dear Branton,

thank you for the valuable comments. I updated the definitions and examples accordingly. Furthermore, I copied the earlier description of atom_site_moment_Fourier_param.cos_symmform and tried adapting it to the other symmform definitions in question. Do I understand correctly, that string mxp1 would never appear as a value of atom_site_moment_Fourier_param.sin_symmform, since for the sin symmform the third character is limit to either s (sin) or c (cos)?

It would also be great if you could review the final pull request #68, just to see if did not mix anything up.

Sincerely
Antanas

@brantonc
Copy link
Collaborator

brantonc commented Feb 6, 2024 via email

@vaitkus vaitkus linked a pull request Feb 21, 2024 that will close this issue
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 a pull request may close this issue.

2 participants