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

Fix import issues #42

Merged
merged 4 commits into from
Sep 11, 2023
Merged

Fix import issues #42

merged 4 commits into from
Sep 11, 2023

Conversation

vaitkus
Copy link
Collaborator

@vaitkus vaitkus commented Jul 19, 2023

This PR fixes the following issues:

  • Corrects the name of the Head save frame from the cif_ms.dic dictionary. Previously, the data block name was incorrectly provided.
  • Add the "Ignore" on duplicate flag to allow the import of save frames from the cif_ms.dic dictionary. This is probably the intended behaviour. Note, that under this mode, even if the definition of a duplicate category is ignored, non-duplicate items from that category are still imported.

Additional things to address:

  • In order for the "Ignore" flag to function as intended, the ATOM_SITE_FOURIER_WAVE_VECTOR category key had to be explicitly specified in this dictionary as well (that part of the definition was copied from the cif_ms.dic dictionary. Maybe it would make sense to also copy the full definition of the _atom_site_Fourier_wave_vector.seq_id data item?
  • The import of the CIF_CORE category is a bit redundant since it is already imported in full by the CIF_MS dictionary.
  • In order for all import statements to function correctly, the import statements in the Modulated Structures dictionary should also be updated (Specify the import mode of the CIF_CORE dictionary Modulated_Structures#11).

Note, that under this import mode the ATOM_SITE_FOURIER_WAVE_VECTOR category
key item must be explicitly specified since the definition in the imported
dictionary is ignored.
@jamesrhester
Copy link
Contributor

I think it would be OK to also remove the cif_core import in this PR. I am not sure about the reasoning behing restating the definition of _atom_site_Fourier_wave_vector.seq_id

@vaitkus
Copy link
Collaborator Author

vaitkus commented Sep 8, 2023

Ok, I removed the CIF_CORE import statement.

My suggestion to redefine _atom_site_Fourier_wave_vector.seq_id in CIF_MAG is mainly to make the key category item more clearly visible. Currently, the item is stated as the key item of ATOM_SITE_FOURIER_WAVE_VECTOR, but the definition of the key itself is provided in a completely different dictionary (CIF_MS). Also, this would make the CIF_MAG slightly more resilient to changes in the CIF_MAG (e.g. removal of the _atom_site_Fourier_wave_vector.seq_id). But this is not really something that we have to definitively decide right now, so feel free to merge the PR without this change.

Finally, I noticed that the ATOM_SITE_FOURIER_WAVE_VECTOR category is defined as being the child of MS_GROUP which is the head category of the imported CIF_MS dictionary. To my understanding, the head category of CIF_MAG should absorb the head category of CIF_MS thus making it no longer visible. Can we change the parent category of ATOM_SITE_FOURIER_WAVE_VECTOR to MAGNETIC or is my interpretation off here?

@vaitkus
Copy link
Collaborator Author

vaitkus commented Sep 8, 2023

Ok, I also just noticed that while the ATOM_SITE_FOURIER_WAVE_VECTOR category is redefined as Loop, the _atom_site_Fourier_wave_vector.seq_id data item is not specified as loop key (actually no items are specified as keys). This is a bug, right?

@jamesrhester
Copy link
Contributor

The atom_site_fourier_wave_vector category issues should be dealt with by a separate PR. There is a definitely a bug in the way it is currently specified, but it's not clear which dictionary (MS/MAG) is most appropriate to deal with it.

@jamesrhester jamesrhester merged commit 916e162 into COMCIFS:main Sep 11, 2023
1 check passed
@vaitkus vaitkus deleted the fix-import-issues branch September 11, 2023 07:09
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