do we really want to allow zero atoms? <di

grammar comments about extended-xyz HOT 2 OPEN

bernstei commented on September 26, 2024

grammar comments

from extended-xyz.

Comments (2)

Luthaf commented on September 26, 2024

The comments are on the previous grammar, the current one is in these files:

https://github.com/Luthaf/extended-xyz/blob/master/exyz-lark/extended-xyz-comment.lark
https://github.com/Luthaf/extended-xyz/blob/master/exyz-lark/extended-xyz.lark

do we really want to allow zero atoms?

I think that's a corner case likely enough to append to make it worth supporting. For example, GCMC simulations at low pressure would have a couple of frames along the "trajectory" with 0 atoms.

allow .5 as a valid float? It should.

Good point, I'll add a case for this

The other comments should be fixed by the lark grammar

where the use of '*' suggests that we allow zero row or column arrays.

extended-xyz/exyz-lark/extended-xyz-comment.lark

Line 31 in 0e740c5

 new_style_array2d: "[" whitespaces? ( new_style_array1d ( whitespaces? "," whitespaces? new_style_array1d )* ) whitespaces? "]" 

I don't see where the ":" separating entries for each property

extended-xyz/exyz-lark/extended-xyz-comment.lark

Line 11 in 0e740c5

 atom_properties_list: "\""? "Properties" "\""? whitespaces? "=" whitespaces? "\""? single_atom_property (":" single_atom_property)* "\""? 

from extended-xyz.

bernstei commented on September 26, 2024

I hadn't thought about GCMC, and I suspect we don't allow it now, but I agree that it's worth supporting. Just need to make sure the number of atoms line is required to be >= 0, rather than > 0.

All the other changes look good.

from extended-xyz.

Recommend Projects