search

COMCIFS / magnetic_dic

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

Added _su entries for Measurand items #78

Closed nautolycus closed 4 months ago

nautolycus commented 5 months ago

Note that I have provided "all_underscore" aliases for these new _su data items, as this seems to be standard practice for all items in this dictionary

nautolycus commented 5 months ago

Looks good to me, thank you.

Although, there is one minor thing that is done slightly differently in other dictionaries as far as the SU item definitions are concerned. Instead of repeating the same attributes in all SU definitions, these attributes are instead imported as the general_su save frame from the templ_attr.cif template file (see for example, https://github.com/COMCIFS/cif_core/blob/00815d6aa0df32560f6ecb3561e428c3e5feb29f/cif_core.dic#L5905).

Not sure if this is relevant for this dictionary though, so I will approve the PR, but I will leave it for @jamesrhester to merge it.

I didn't notice this. But some of the items I have added have _type.container Matrix, so the general_su import won't apply to them.

vaitkus commented 5 months ago

I didn't notice this. But some of the items I have added have _type.container Matrix, so the general_su import won't apply to them.

Indeed. There were a few similar instances in the CIF_CORE as well which had to be handled individually.

vaitkus commented 4 months ago

@nautolycus It is a bit unfortunate that you deleted the PR branch, since it was OK overall and only needed some minor tweaks to be merged (addition of some import statements). Maybe you can restore the branch via GitHub or still have a local copy on your machine that could be pushed? Or do you have some different plans, e.g. to start from scratch in a new branch?