COMCIFS / magnetic_dic

Development of the magnetic CIF dictionary
0 stars 4 forks source link

Fix import issues #42

Closed vaitkus closed 11 months ago

vaitkus commented 1 year ago

This PR fixes the following issues:

Additional things to address:

jamesrhester commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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 commented 11 months ago

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.