-
Notifications
You must be signed in to change notification settings - Fork 9
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
index into a default list with more than one key value (ie create ENUMERATION_DEFAULTS
)
#399
index into a default list with more than one key value (ie create ENUMERATION_DEFAULTS
)
#399
Conversation
An example use would be:
* |
I am thinking that it might be better to create a completely new category that would be most similar to @rowlesmr , @jamesrhester what are your opinions on this? |
That makes some sense. The errors in the ddlm check as (I think) associated with not having the category key present, even though it's populated through dREL. Something like:
. The alternative would be to keep the current categories and just add . More thinking, from #397 (comment)
I'm an idiot. We need @vaitkus 's solution above, as we are going to be changing the category key. But I would combine it with @jamesrhester 's preference, and deprecate the old and change over all the existing to the new. I'll put something together. |
Just some other things to think about: The atom symbol is a concatenation of element symbol and charge. If redesigning the entire system (which we're not), this could be changed to either separate element and charge or to also include the isotope number (if not natural abundance). i) Separating the charge from the element would make the atomic mass, atomic number, radius bond*, dispersion tables, and the like much simpler. This would necessitate the requirement to index multiple keys to reference scattering factors and the like. ii) Adding the isotope number to the and then (the position which should be chosen), iii) Dont't touch * why are the bond radii of ions and neutral atoms the same? Whence did they come? |
My thinking is as follows: If we create a new category to supercede
Either of these approaches is viable, which means the decision is a policy one. I'm inclined to go with the update of DDLm to version 5.0, and simply provide the new category key with the dREL that explains how it is derived from the old category key. My reasons for preferring this is that at this level, very few software authors are affected (probably all are reading this message) and it keeps the foundational language cleaner. If going with the above approach, we would need to pass it by ddlm-group for policy signoff. |
I have no intuition as to the best approach, so I'll go with the nuclear option of just changing the existing category. If proposing that to the dlm-group, then I don't think I need to do anything more.* I've also already updated * I can't really update the scattering length, as that also needs a new data item, and that would complicate things in this PR. |
@jamesrhester As you mentioned, there seems to be two conflicting policies here. From my point of view, the proposed approach does not seem cleaner since it breaks things for DDLm software that is not capable of dynamically interpreting dREL code. Of course, the coders can still use dREL code as a reference, but it does require going back and tweaking the software. Given that, software from the I also understand that having two categories which serve a similar purpose may be somewhat confusing though still a much lesser evil from a programmers point of view as the handling of the new category can be implemented completely independently. If the ddlm-group will prefer to go with the "nuclear" solution, I apologize in advance if the |
@vaitkus , if the Also, if I am not wedded to the "nuclear" option, but the other option would mean an old, unused category that would ideally eventually be removed, leading again to a version 5 of DDLm. |
I think that would work. I would eventually implement the checking of the new relationships.
This is indeed something worth implementing since it would at least help to detect the incompatibilities. However, the underlying need to modify the code would still remain.
I think that the accumulation of some outdated or deprecated definitions is unavoidable. These should preferably still be supported for a few intermediate versions and then eventually removed with the next major release (or even the next after that). Further more, is I correctly understand the current proposal, it still leaves some deprecated items ( However, I do understand that this version of DDLm can still potentially be considered early in development so some less conservative changes may be preferred. Are these changes something you would like to see published in the upcoming volume G of ITC or are they aimed at a future release? As a side note, the new propose name |
@vaitkus makes a good argument. I say go with this. It still ends in a v5 with things removed, but is a more gradual, and officially deprecates them prior to removal. Side note: is there anything in the CIF release notes about communicating the actual removal of deprecated dataitems?
I am definitely not wedded to |
OK, I'm happy to go with the preference of people who are actually using this thing. Let's have a new category and leave things at DDLm version 4 for now. As this is in time for Volume G 2nd Edition, I will write it up in place of the old data names. What do we think about |
Apart from removing the dREL I think I'm happy to merge this. What do you think @vaitkus ? |
I shall remove the drel. I think I put it there to avoid having to alter all the enumerations in templ_enum, but they shouldn't be too hard to convert; I'll have to find where I stashed my go at changing them. |
as per comments from @jamesrhester
ENUMERATION_DEFAULTS
)
This change can be independent of any changes in |
@jamesrhester There is actually one annoying issue with Possible solutions:
2.a Alternatively, we could continue having a single content type 2.b Same as 2a, but we also state that the values are normalised before converting them to the |
Yes, this type of indexing is something we have deliberately removed from the main dictionary, which wanted to index all categories using a single data name potentially created from several data values. We can't do that here without redesigning the entire default system. We want a low-effort solution, as all we are talking about is looking up default values to inform calculations of derived values by dREL. This is not something that affects actual data transmission or archiving. So some thoughts:
So I would propose simply defining an "Used only with compound data values in the DDLm attribute dictionary. The data type of each value in a compound value is the same as that of the data name to which it is directly related." |
Would you like me to work that into this PR? |
Let's wait and see what @vaitkus thinks. |
I am generally OK with having the However, the relationship between the value and the related item might not always be clear to an outside reader, so the definition of In the next major revision we may come up with better ideas, but, as you mentioned, this would require a serious reworking of the typing system. |
I've added some words. The definition of |
I got it wrong, a Does
mean |
Not sure if you can multiply two arrays like this. My interpretation would be more like |
Sure you can. It's element-wise multiplication (Hadamad product). |
Just to confirm, yes, An A |
I think this is ready for merging if those few suggested changes above are incorporated, and the minor version is bumped in |
@jamesrhester The last version number change already increased the minor version, that is, the version was changed from 4.0.2 to 4.1.0, so the DDL version number increase is probably not necessary. Or should we create a new version of 4.2.0 and put all the changes from this branch there? Also, should we formally deprecate the old ENUMERATION_DEFAULT category? |
Yes, we should deprecate the old category. OK not to change the version number then, as long as it is different to the one used in the 3.2.0 core release. |
Ok, so I did quite a few changes since the last review:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it looks all good. Just that one change on line 1002.
Co-authored-by: Matthew Rowles <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The validator does not yet properly handle the 'Inherited' type, but I updated the GitHub actions to ignore the warnings for now so we can start using these dictionary attributes in other feature branches.
I will most likely implement the proper handling in the upcoming few weeks.
From #397
This is a first attempt at putting things into place.
The idea is to allow indexing a single default value based on an arbitrary number of keys. In the example below, neutron scattering lengths are keyed on element symbol and atomic mass number. In any case, a single value is given to each combination of keys.
The definitions need work.
.
I've put in
_enumeration.def_index_ids
with a default value of[ _enumeration.def_index_id ]
, to give backward compatibility with the current use; in the new scheme, you need to parse_enumeration.def_index_ids
, and we don't need to redefine every single current default enumeration.I've generalised
_enumeration_default.index
as_enumeration_default.indexes
, even though may not make total sense to make it a plural. Again, in the new scheme,_enumeration_default.indexes
has the default value of[ _enumeration_default.index ]
, to give backward compatibility with the current use