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

Added _su entries for Measurand items #78

Closed
wants to merge 3 commits into from

Conversation

nautolycus
Copy link
Contributor

@nautolycus nautolycus commented Mar 15, 2024

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 nautolycus marked this pull request as draft March 15, 2024 10:16
@nautolycus nautolycus marked this pull request as ready for review March 15, 2024 10:41
cif_mag.dic Outdated Show resolved Hide resolved
cif_mag.dic Show resolved Hide resolved
Copy link
Collaborator

@vaitkus vaitkus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@vaitkus vaitkus linked an issue Mar 15, 2024 that may be closed by this pull request
@nautolycus
Copy link
Contributor Author

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
Copy link
Collaborator

vaitkus commented Mar 15, 2024

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.

Copy link
Contributor

@jamesrhester jamesrhester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that we should use the general_su import for su data names that are not matrices, to save a little space and keep consistency. The magnetic dictionary already uses imports, so we are not introducing any more complexity either.

@nautolycus nautolycus closed this Apr 3, 2024
@nautolycus nautolycus deleted the standard_uncertainties branch April 3, 2024 09:20
@vaitkus
Copy link
Collaborator

vaitkus commented Apr 3, 2024

@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?

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.

Add missing SU data items
3 participants