Explicitly declare relations between 'elements', 'nelements' and 'elements_ratios'

[merkys]

Reading the specification once more I find the relations between elements, nelements and elements_ratios lacking explicit declaration:

1. Lists of elements and elements_ratios are correlated;
2. nelements is equal to lengths of elements and elements_ratios.

These relations can be derived from the examples, or the general knowledge, but I believe it would be clearer to have them written explicitly.

[ml-evs]

Thanks @merkys, I think this is a useful clarification.

[ml-evs]

Did anyone else want to take a look at this before 1.0.1? @rartino/@CasperWA?

[CasperWA]

Two things:

• I am not completely comfortable with the "correlated" sentences, as the "correlation" is not defined.
• I think we should make clear that the MUST requirement between properties are only valid as long as both properties are given, i.e., not null.
  This could also be done via an introductory sentence at the top of the section, stating something like "all inter-property requirements are only valid as long as both properties are provided, i.e., neither have an unknown value"?

[merkys]

• I am not completely comfortable with the "correlated" sentences, as the "correlation" is not defined.

As @ml-evs wrote, term "correlation" was already in use in the specification of v1.0.0. In addition to use in section for filtering, it is also mentioned in Structures entries section to emphasize such relation between various lists.

The situation could be possibly improved by describing "correlated lists" in the definition of terms section. Maybe it could also cite Wikipedia page on parallel array which describes the idea pretty well IMO. Maybe we could even switch to use "parallel lists" if this term is more intuitive/used more widely.

• I think we should make clear that the MUST requirement between properties are only valid as long as both properties are given, i.e., not null.
  This could also be done via an introductory sentence at the top of the section, stating something like "all inter-property requirements are only valid as long as both properties are provided, i.e., neither have an unknown value"?

I agree, this would bring some more clarity to the specification. Of course, first we have to make sure whether this is a truth global to the specification :)

[CasperWA]

So nelements only has to be equal length but not refer to the other fields with the same order and elements and elements_ratios does not have to have equal length to the others? 😅

[merkys]

So nelements only has to be equal length but not refer to the other fields with the same order and elements and elements_ratios does not have to have equal length to the others? sweat_smile

Oops, will change this as well!

[rartino]

I like this version; this way of expressing the "correlation" criteria is fully clear to me.

[CasperWA]

So nelements only has to be equal length but not refer to the other fields with the same order and elements and elements_ratios does not have to have equal length to the others? sweat_smile

Oops, will change this as well!

Did you implement these changes yet? I don't think I've seen it yet. I can do it though? :)

[merkys]

So nelements only has to be equal length but not refer to the other fields with the same order and elements and elements_ratios does not have to have equal length to the others? sweat_smile

Oops, will change this as well!

Did you implement these changes yet? I don't think I've seen it yet. I can do it though? :)

I just fixed them. Sorry for the delay.

[rartino]

Sorry for doing this... Would this be shorter and even more clear (?) written as this:

Suggested change

	- MUST be of equal length as `elements\_ratios` and refer to the elements in the same order as `elements\_ratios`_, if the latter is provided.
	- MUST refer to the same elements in the same order as `elements\_ratios`, if the latter is provided.

[CasperWA]

This is close to what he had before (i.e., before 263a303), where I asked him to add the length part as well, simply to be as explicit as possible. However, I guess it could work this way as well when same has been added. I am OK with either, but would personally prefer the more explicit edition of the two here - so I'd leave this up to @merkys to choose your personal favourite :)

[rartino]

No... @CasperWA, I think you miss the formal difference of the addition of an extra "same" in the sentence: "MUST refer to the elements in the same order as elements_ratios" vs. "MUST refer to the same elements in the same order as elements_ratios".

I'm arguing that latter implies that the lengths must be equal and is possibly even stronger. (Just saying that the lengths must be equal and the order the same could allow referring to the same element twice and leaving another one out...)

[ml-evs]

I have just fixed the conflict that I added inadvertently in #358, and I am happy with this wording.

[shyamd]

I'll throw a wrench into the discussion.
nelements should equal len(elements), this is fine.
In an experiment with known impurities, a DB might want to list those impurities but not include them in the element_ratios, so then the list length would not match between elements and element_ratios

[ml-evs]

I'll throw a wrench into the discussion.
nelements should equal len(elements), this is fine.
In an experiment with known impurities, a DB might want to list those impurities but not include them in the element_ratios, so then the list length would not match between elements and element_ratios

This is a good point that could do with some clarification (I would hope in another PR ;)). The statistical aspects of compositions are handled by species and species_at_sites, but there is currently no linkage from species_at_sites to elements, e.g. we do not require that set(symbol for species in entry.attributes.species for symbol in species.chemical_symbols) == entry.attributes.elements. Of course, impurity could mean 10% or it could mean 0.00001%...

I would suggest that given the existing interpretation (or lack thereof) of the specification, we should still merge this pending further discussion. This requirement has been implicit (and is coded up in the python-tools models, for example); if we decide to relax it based on this impurity use-case then that will not be a breaking change.

[CasperWA]

Let's get this in. It's been discussed extensively and I agree with the current edition. Cheers @merkys ! :)

[rartino]

Oh, didn't we merge this yet? I approve the present version.

[merkys]

Thanks for approving and merging this!

[ml-evs]

I have just opened #361 to carry on @shyamd's discussion point