Atomistic file extension #16

MSallermann · 2021-08-19T14:04:18Z

Implements the format suggested in #15.

Includes a partial refactor of the parsing of the segment header keywords:

keyword/value pairs are now specified as their own pegtl rules, see keywords.hpp and atomistic_keywords.hpp
- This allows one to be flexible regarding the shapes etc., which is useful for e.g for multiline keywords like the new 'basis'
- Also makes it possible to transparently select, which keyword value pairs are allowed for which file format
Validation of keywords with regards to the mesh has been moved to a single pass, once the header is read.
Overall the refactor ended up quite verbose ... suggestions welcome.

Then the fileformat discussed in the issue has been implemented with some minor changes.

For the basis I chose the format:

# basis:
# 0 0 0
# 0.5 0 0

In compatibility mode, the version string reads:

# OOMMF OVF 2.0
##% AOVF_COMP 1.0

The lattice meshtype needs to be selected with an additional: #%% meshtype lattice, which overwrites any other meshtype
Extensive unit tests for the new format have been added.
On reading in a file, the fileformat is automatically detected and saved in a new member of ovf_file : ovf_extension_format.

 /* OVF file formats */
#define OVF_EXTENSION_FORMAT_OVF                0 // Standard OVF 2 format
#define OVF_EXTENSION_FORMAT_AOVF              1 // Atomistic extension, with new keywords
#define OVF_EXTENSION_FORMAT_AOVF_COMP  2 // Atomistic extension in compatibility format, (##% prefix for new keywords)

On writing a file, having set meshtype lattice will lead to an error if OVF_EXTENSION_FORMAT_OVF is used.

TODO

Fortran bindings
Maybe squash some commits to remove noise from the log
Formatting
General pass to clean up some rough edges
Check if performance has suffered
Check all edge cases, where unexpected behaviour may occur
Naming of some files variables should probably be improved
Bravais and basis vectors should probably be saved as doubles not floats
When using the plain OVF format ##% lines are not ignored

Improved pegtl rules for keywords.

… cnodes members

coveralls · 2021-08-19T14:06:41Z

Coverage increased (+1.4%) to 94.286% when pulling 32da6aa on MSallermann:atomistic_v2 into 6e3383e on spirit-code:master.

codecov · 2021-08-19T14:06:55Z

Codecov Report

Merging #16 (32da6aa) into master (6e3383e) will increase coverage by 0.15%.
The diff coverage is 77.58%.

@@            Coverage Diff             @@
##           master      #16      +/-   ##
==========================================
+ Coverage   52.22%   52.37%   +0.15%     
==========================================
  Files           8       10       +2     
  Lines        1779     1814      +35     
==========================================
+ Hits          929      950      +21     
- Misses        850      864      +14

GPMueller

Looks mostly good so far

GPMueller · 2021-08-19T16:33:32Z

include/detail/atomistic_keywords.hpp

+    ////// bravaisa
+    // x
+    struct bravaisa_value_x : pegtl::pad<ovf::detail::parse::decimal_number, pegtl::blank>
+    { };
+
+    template<>
+    struct kw_action< bravaisa_value_x >
+    {
+        template< typename Input >
+        static void apply( const Input& in, ovf_file & f, ovf_segment & segment)
+        {
+            segment.bravaisa[0] = std::stof(in.string());
+        }
+    };
+
+    // y
+    struct bravaisa_value_y : pegtl::pad<decimal_number, pegtl::blank>
+    { };
+
+    template<>
+    struct kw_action< bravaisa_value_y >
+    {
+        template< typename Input >
+        static void apply( const Input& in, ovf_file & f, ovf_segment & segment)
+        {
+            segment.bravaisa[1] = std::stof(in.string());
+        }
+    };
+
+    // z
+    struct bravaisa_value_z : pegtl::pad<decimal_number, pegtl::blank>
+    { };
+
+    template<>
+    struct kw_action< bravaisa_value_z >
+    {
+        template< typename Input >
+        static void apply( const Input& in, ovf_file & f, ovf_segment & segment)
+        {
+            segment.bravaisa[2] = std::stof(in.string());
+        }
+    };
+
+    struct bravaisa_value : pegtl::seq<bravaisa_value_x, bravaisa_value_y, bravaisa_value_z, end_kw_value>
+    { };
+
+    template<>
+    struct kw_action< bravaisa_value >
+    {
+        template< typename Input >
+        static void apply( const Input& in, ovf_file & f, ovf_segment & segment)
+        {
+            f._state->found_bravaisa = true;
+        }
+    };
+
+    struct bravaisa : TAO_PEGTL_ISTRING("bravaisa")
+    { };


why split the vector into distinct rules instead of reading in a vector-3?

GPMueller · 2021-08-19T16:41:30Z

include/detail/parse.hpp

        else if( file.version == 1 )
        {
-            // TODO...
-            file._state->message_latest = fmt::format(
-                "libovf segment_header: OVF version \'{}\' in file \'{}\' is not supported...",
-                file.file_name, file.version);
-            return OVF_INVALID;
+            if(file.ovf_extension_format == OVF_EXTENSION_FORMAT_AOVF)
+                success = pegtl::parse< pegtl::plus<v2::segment_header<v2::aovf_keyword_value_line>>, v2::ovf_segment_header_action, v2::ovf_segment_header_control >( in, file, segment );
+            else if (file.ovf_extension_format == OVF_EXTENSION_FORMAT_AOVF_COMP)
+                success = pegtl::parse< pegtl::plus<v2::segment_header<v2::caovf_keyword_value_line>>, v2::ovf_segment_header_action, v2::ovf_segment_header_control >( in, file, segment );


isn't it confusing if the extension formats are discerned here, inside if( file.version == 1), as they are an extension of the version 2 format?

GPMueller · 2021-08-19T17:11:43Z

include/detail/write.hpp

        output_to_file += fmt::format( "# meshtype: {}\n", meshtype );

        int n_rows = 0;
-        if( meshtype == "rectangular" )
+        if( meshtype == "rectangular" && std::string(segment->meshtype) != "lattice")


the second half should be just meshtype != "lattice"

GPMueller · 2021-08-19T17:48:56Z

test/atomistic.cpp

+        segment->title = const_cast<char *>("ovf test title - append");
+        segment->comment = const_cast<char *>("test append");


const_casting here could yield undefined behaviour in other places, see https://en.cppreference.com/w/cpp/language/string_literal
It is used here in Spirit as well...
I'm not sure what the right way to assign to these char *s would be in C++...

Seems like a wrapper like this might be the way to go:

template <size_t N> char * ptr_from_literal(const char (&str)[N]) { char* ptr = new char[N]; memcpy(ptr, str, N); return ptr; }

(got it from https://www.py4u.net/discuss/101381 )

In fact, if we want to avoid a small memory leak in Spirit, we need to

if( segment->title ) delete segment->title; ....

etc before each assignment to any of the char *s.

…alid ovf and avof files.

…d empty lines

- magic "%" char now only changes behaviour in AOVF_COMP format - parse_rules are now templated with a `Version` enum - Some accompanying changes in parse

- simplified parsing bravais vectors in atomistic_keywords with it

Replaced it with strdup("literal") throughout the code.

GPMueller

In several files, you updated the formatting. Please apply clang-format to all sources (including tests) to make it consistent again.

Regarding the naming, I find it hard to say whether it would be better to go with the classical ovf-style naming for the new aovf keywords, or to start using "nicer" names.
If you want to provide an incompatible aovf format, it might be worth considering defining an entirely new, consistent set of keywords.

The changes are too many to really review, so I hope your tests are thorough enough ;)

GPMueller · 2021-09-06T18:58:31Z

include/detail/atomistic_keywords.hpp

        }
    };

-    struct bravaisa_value : pegtl::seq<bravaisa_value_x, bravaisa_value_y, bravaisa_value_z, end_kw_value>
+    ////// bravaisa
+    struct bravaisa : TAO_PEGTL_ISTRING("bravaisa")


maybe "bravais_a" would be a more readable name than "bravaisa"?

GPMueller · 2021-09-06T19:01:05Z

include/detail/atomistic_keywords.hpp

-    struct bravaisc : TAO_PEGTL_ISTRING("bravaisc")
-    { };
-
-
    ////// ncellpoints
    struct ncellpoints : TAO_PEGTL_ISTRING("ncellpoints")


maybe "n_cell_points" would be more readable?

GPMueller · 2021-09-06T19:15:00Z

include/detail/atomistic_keywords.hpp

+            float temp[3];
+            read_vector( in, temp );
+            f._state->_basis.push_back( {temp[0], temp[1], temp[2]} );


ovf/include/detail/pegtl_defines.hpp

Line 61 in 32da6aa

std::vector<std::array<float, 3>> _basis = std::vector<std::array<float, 3>>(0);

I therefore believe it would be more canonical to write

auto atom = f._state->_basis.emplace_back(); read_vector( in, (*atom).data() );

or

std::array<float,3> atom; read_vector( in, atom.data() ); f._state->_basis.push_back(atom);

GPMueller · 2021-09-06T19:19:16Z

include/detail/atomistic_keywords.hpp

            f._state->found_basis = true;
-            if( segment.ncellpoints != f._state->_cur_basis_line ) // Need to make sure that the basis array is already allocated
+            // Allocate and data in segment struct and copy
+            segment.basis = new float[3 * f._state->_basis.size()];


To avoid memory leaks, lines like this should probably be preceded by

if( segment.basis ) delete segment.basis;

GPMueller · 2021-09-06T21:13:38Z

include/detail/parse.hpp

@@ -70,7 +70,7 @@ namespace parse
        if( success )
        {
            success = false;
-            if( file.version == 2 )
+            if( file.version == 2 || (file.version == 1 && (file.ovf_extension_format == OVF_EXTENSION_FORMAT_AOVF || file.ovf_extension_format == OVF_EXTENSION_FORMAT_AOVF_COMP )))


shouldn't the OVF_EXTENSION_FORMAT_AOVF_COMP case be for file.version == 2?

if( file.version == 2 || (file.version == 2 && file.ovf_extension_format == OVF_EXTENSION_FORMAT_AOVF_COMP) || (file.version == 1 && file.ovf_extension_format == OVF_EXTENSION_FORMAT_AOVF) )

Moritz added 20 commits August 18, 2021 14:55

Refactor: Moved validation of segment header keywords

3432729

Moved parser_state into its own header

6fef06f

Added keywords header file

b582197

Use pegtl keywords rules to parse segment headers.

f4fcb5d

Improved pegtl rules for keywords.

Implemented atomistic keywords

fcee71d

Improved exceptions in atomistic keywords

e8346d1

Implemented parsing of atomsitic keywords

f3f0257

Adjusted ctypes structs to atomistic properties

b8b3d63

Added unit test for atomistic ovf and made it work

b6005d6

Added check for missing and wrong keywords for meshtype 'lattice'

0f19e98

Use segment.n_cells member instead of creating new anodes, bnodes and…

27ddbb2

… cnodes members

Minor changes in parser_state

721a632

Implemented compatibility format in write.

b2fc5e3

Improved implementation of basis_value keyword

b6d3c21

Adjusted keyword basis for aovf_comp format

e117b62

Completed parsing/writing of AOVF_COMP format files

6c61daf

Testing: Moved atomistic tests to own file and extended them

7f94b2a

Small fix for atomistic keywords meshtype value

97adfad

Added extension_format defines in ovf.py

29f3b2c

Added simple test for atomistic case to python API

3d96f05

MSallermann requested a review from GPMueller August 19, 2021 14:06

GPMueller reviewed Aug 19, 2021

View reviewed changes

Moritz added 6 commits August 27, 2021 08:41

Unit test: Added a test that tries to read in weirdly formatted but v…

b33696d

…alid ovf and avof files.

Parse_Rules: Changes to be more robust against whitespace/comments an…

9a5a338

…d empty lines

Parse_Rules: More rigorous parsing

8f5e068

- magic "%" char now only changes behaviour in AOVF_COMP format - parse_rules are now templated with a `Version` enum - Some accompanying changes in parse

Moved pegtl parsing of Vector3 into a utility function

9b3ddcc

- simplified parsing bravais vectors in atomistic_keywords with it

Renamed found_meshtype_atomistic to found_meshtype_lattice

6c1308b

Removed const_cast<char*>("literal").

32da6aa

Replaced it with strdup("literal") throughout the code.

MSallermann force-pushed the atomistic_v2 branch from f365464 to 32da6aa Compare September 5, 2021 20:31

GPMueller requested changes Sep 6, 2021

View reviewed changes

GPMueller reviewed Sep 6, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Atomistic file extension #16

Atomistic file extension #16

MSallermann commented Aug 19, 2021 •

edited

coveralls commented Aug 19, 2021 •

edited

codecov bot commented Aug 19, 2021 •

edited

GPMueller left a comment

GPMueller Aug 19, 2021

GPMueller Aug 19, 2021

GPMueller Aug 19, 2021

GPMueller Aug 19, 2021

GPMueller left a comment

GPMueller Sep 6, 2021

GPMueller Sep 6, 2021

GPMueller Sep 6, 2021

GPMueller Sep 6, 2021

GPMueller Sep 6, 2021 •

edited

		segment->title = const_cast<char *>("ovf test title - append");
		segment->comment = const_cast<char *>("test append");

Atomistic file extension #16

Are you sure you want to change the base?

Atomistic file extension #16

Conversation

MSallermann commented Aug 19, 2021 • edited

coveralls commented Aug 19, 2021 • edited

codecov bot commented Aug 19, 2021 • edited

Codecov Report

GPMueller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GPMueller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GPMueller Sep 6, 2021 • edited

Choose a reason for hiding this comment

MSallermann commented Aug 19, 2021 •

edited

coveralls commented Aug 19, 2021 •

edited

codecov bot commented Aug 19, 2021 •

edited

GPMueller Sep 6, 2021 •

edited