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

Add changes from the 'modulated-development' branch (CIF_CORE) #3

Merged

Conversation

vaitkus
Copy link
Contributor

@vaitkus vaitkus commented Jul 18, 2023

This PR includes changes taken from the 'modulated-development' branch [1] and adapted to the latest version of the dictionary in this repository. I am not sure if any of those changes are still relevant, but I wanted to address them before making any more significant changes to the dictionary. From what I can tell from the log messages of the original branch, the changes in question were originally suggested by G. Madariaga.

The default Git diff does quite a bad job of properly conveying the changes (e.g. it shows significant changes to existing data items instead of addition of new data items), therefore it might be more convenient to use the simple (*nix) diff command to compare the dictionary in this branch with the dictionary in the master branch.

Changes taken from the CIF_CORE branch include the addition of new data items and categories, addition of new dREL code (related to the added items) and the change of the ATOM_SITE_U_FOURIER category key.

The list of added categories and items:

  • ATOM_SITE_DISPLACE_LEGENDRE_PARAM
  • ATOM_SITE_DISPLACE_ORTHO_PARAM
  • _atom_site_displace_special_func.zigzag_ax
  • _atom_site_displace_special_func.zigzag_ay
  • _atom_site_displace_special_func.zigzag_az
  • ATOM_SITE_DISPLACE_XHARM_PARAM
  • _atom_site_Fourier_wave_vector.q[1-8]_coeff and atom_site_Fourier_wave_vector.q[1-8]_coeff_seq_id
  • ATOM_SITE_OCC_LEGENDRE_PARAM
  • ATOM_SITE_OCC_ORTHO_PARAM
  • ATOM_SITE_OCC_XHARM_PARAM
  • ATOM_SITE_ROT_LEGENDRE_PARAM
  • ATOM_SITE_ROT_ORTHO_PARAM
  • _atom_site_rot_special_func.zigzag_ax
  • _atom_site_rot_special_func.zigzag_ay
  • _atom_site_rot_special_func.zigzag_az
  • ATOM_SITE_ROT_XHARM_PARAM
  • ATOM_SITE_U_LEGENDRE_PARAM
  • ATOM_SITE_U_ORTHO_PARAM
  • ATOM_SITE_U_XHARM_PARAM
  • atom_sites_axes.matrix_*
  • _atom_sites_ortho.coeff_cos
  • _atom_sites_ortho.coeff_sin

Also, it should be noted, that the original branch did not (yet) contain some of the items that are now included in the dictionary, so it is possible that they were either replaced by or added instead of the new items and categories in this PR,
e.g. atom_site_displace_Legendre.coeff instead of ATOM_SITE_DISPLACE_LEGENDRE_PARAM category. This PR does NOT remove these items. Items in question:

  • _atom_site_displace_Legendre.coeff
  • _atom_site_displace_ortho.coeff
  • _atom_site_displace_xharm.coeff
  • _atom_site_occ_Legendre.coeff
  • _atom_site_occ_ortho.coeff
  • _atom_site_occ_xharm.coeff
  • _atom_site_U_Fourier.id
  • _atom_site_U_Legendre.coeff
  • _atom_site_U_ortho.coeff
  • _atom_site_U_xharm.coeff

The original branch also included some changes that were NOT included in this PR:

  • The purpose of some items was changed. However, the majority of those changes were already superseded by other commits as well as changes in the overall semantics of the _type.purpose attribute. If needed, I can include those in the PR as well, but at this point is probably be more convenient to just go over the entire item list again and double checks if anything seems out of order.
  • The addition of of undotted aliases (e.g. _atom_site_occ_xharm_order). However, those aliases do not seem to have been previously defined thus I chose not to include them.

[1] https://github.com/COMCIFS/cif_core/tree/modulated-development

@jamesrhester, maybe you recall the purpose behind the original branch and if it is still relevant? Also, should I include the changes to the Item purposes and addition of alias or is it OK to leave them out?

…epository.

Changes taken from the branch include the addition of new data items and
categories, addition of new dREL code and the change of the ATOM_SITE_U_FOURIER
category key.

Item purpose changes as well as addition of undotted aliases were not included.

[1] https://github.com/COMCIFS/cif_core/tree/modulated-development
The flag was added accidentally while testing and was not included in the original branch.
@jamesrhester
Copy link
Contributor

Thank you @vaitkus for this work. I have checked my emails, and this PR should be merged, as the changes relate to a new ms dictionary draft provided to me by G. Madariaga in 2017. Once it is merged we can delete the previous branch in cif core. There may be outstanding changes from the 2017 draft, but I think I can provide them as incremental updates once the current ones have been merged.

@vaitkus vaitkus marked this pull request as ready for review September 8, 2023 09:08
@vaitkus
Copy link
Contributor Author

vaitkus commented Sep 8, 2023

Ok, I integrated the latest changes from the main branch so the PR is ready to be merged.

@jamesrhester jamesrhester merged commit 25d97af into COMCIFS:main Sep 8, 2023
@vaitkus vaitkus deleted the add-updates-from-cif-core-repo-branch branch September 8, 2023 10:58
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.

2 participants