Replace rapidjson to nlohmann json #58

RiccardoRossi · 2017-02-21T11:30:11Z

Hello,
this branch removes rapidjson as support library for the KratosParameters and replaces it to the "nlohmann" json library.

This is more or less a complete rewrite of the kratos_parameters, which however maintains the same API and should (hopefully) not affect at any user. It passes all of the existing unit tests.

i believe that the new code base is sensibly cleaner than the old version and the nlohmann library is more modern than rapidjson, apart for being contained within a single header.
The most important advantage is that json objects are stored in a std::map instead in a list as happend with rapidjson, making unordered insertion/search much faster.

A nice feature is also that the new lib should also allow serializing/reading to a binary format

WARNING: the current version has not been tested on windows, so before accepting the pull request someone should try that out.

…ot yet to be merged

…om/KratosMultiphysics/Kratos into replace-rapidjson-to-nlohmann-json

the branch is almost ready for being merged, however an issue with the printing of doubles still remains. Waiting for feedback from the nlohmann library author, who acknowledged the bug

…ting in python

…nto replace-rapidjson-to-nlohmann-json

…into replace-rapidjson-to-nlohmann-json

loumalouomega · 2017-02-22T10:20:06Z

Just one thing, if JSON is faster as IO there is any plan in the future to switch the mdpa in plain text to mdpa.json? @RiccardoRossi @pooyan-dadvand

RiccardoRossi · 2017-02-22T11:51:10Z

Hi, first of all i would like to tell that the use we make of json for Parameters has a lot of error checking which makes it slower than native json. In particular i do like the option of reading directly from json, however this is a very large change and can not be done lightly. Around one year ago i did put a proposal on how this could be done, but it didn't prosper. Eventually we could retake it sometimes cheers Riccardo <http://www.cimne.com/>

pooyan-dadvand · 2017-02-22T12:42:14Z

I will check it in windows. But would be tomorrow.

josep-m-carbonell · 2017-02-22T20:37:32Z

I think two important things must be solved before replacing the json library:

1.- In this library, are list iterators available? And can them be exported to python to avoid asking for the size of the json lists and to avoid loops with range ?

2.- Does it allows to ask for the json keys ? We need this functionality to read the variables in the new materials processes in json.

Regards

RiccardoRossi · 2017-02-22T21:13:04Z

In principle it provides the same capabilities and limitations of the old one. In particular the iterators are behaving exactly identical as they did, and you can take a look to the test_kratos_parameters.py for their usage. Am i missing smthg?

…

On Wed, Feb 22, 2017 at 9:37 PM, Josep Maria ***@***.***> wrote: I think two important things must be solved before replacing the json library: 1.- In this library, are list iterators available? And can them be exported to python to avoid asking for the size of the json lists and to avoid loops with range ? 2.- Does it allows to ask for the json *keys* ? We need this functionality to read the variables in the new materials processes in json. Regards — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#58 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHr7EUOxpkYRDu2MoiS5onu4orvrZy3iks5rfJyMgaJpZM4MHMnJ> .

-- *Riccardo Rossi* PhD, Civil Engineer member of the Kratos Team: www.cimne.com/kratos Tenure Track Lecturer at Universitat Politècnica de Catalunya, BarcelonaTech (UPC) Full Research Professor at International Center for Numerical Methods in Engineering (CIMNE) C/ Gran Capità, s/n, Campus Nord UPC, Ed. B0, Despatx 102 (please deliver post to the porters of building C1) 08034 – Barcelona – Spain – www.cimne.com - T.(+34) 93 401 56 96 skype: *rougered4* <http://www.cimne.com/> <https://www.facebook.com/cimne> <http://blog.cimne.com/> <http://vimeo.com/cimne> <http://www.youtube.com/user/CIMNEvideos> <http://www.linkedin.com/company/cimne> <https://twitter.com/cimne> Les dades personals contingudes en aquest missatge són tractades amb la finalitat de mantenir el contacte professional entre CIMNE i voste. Podra exercir els drets d'accés, rectificació, cancel·lació i oposició, dirigint-se a cimne@cimne.upc.edu. La utilització de la seva adreça de correu electronic per part de CIMNE queda subjecte a les disposicions de la Llei 34/2002, de Serveis de la Societat de la Informació i el Comerç Electronic. Imprimiu aquest missatge, només si és estrictament necessari. <http://www.cimne.com/>

josep-m-carbonell · 2017-02-22T23:20:17Z

Yes, I think something is missing. In the py file you mention, you have the example:

This is how a loop is done now when we have a list in json:

    my_list = subparams["list_value"]
    for i in range(my_list.size()):
        if my_list[i].IsBool()
        ...

and this is how we would like to do it:

    my_list = subparams["list_value"]

    for item in my_list:
        if item.IsBool():
        ...

And also the important feature of getting the keys and variables from a json. As is done in the read_materials_process.py, which imports the native json from python, i.e.:

    #TODO: change to use the KratosParameters once dictionary iterators are exported
    import json

    #read variables 
    for key, value in mat["Variables"].items():
        var = self._GetItemFromModule(key)
        prop.SetValue( var, value)

    #read table
    for key, table in mat["Tables"].items():
        table_name = key

        input_var = self._GetItemFromModule(table["input_variable"])
        output_var = self._GetItemFromModule(table["output_variable"])

        new_table = PiecewiseLinearTable()
        for i in range(len(table["data"])):
            new_table.AddRow(table["data"][i][0], table["data"][i][1])
        prop.SetTable(input_var,output_var,new_table)

I think we also would like to do it without importing json from python.. or can we import it and use it?

RiccardoRossi · 2017-02-23T14:18:14Z

hello,
i added the iterators to the new branch,
you can find an example of usage at the very end of the test_kratos_parameters.py,
(the interface is as the one you described)

however before changing the materials input i would wait for this branch to be approved, since this feature is not current available in the master branch

pooyan-dadvand

I have checked it in my Visual Studio and it doesn't compile due to the fact that my VS build version (14.023107.0) is older than minimum version supported by nlohmann json (14.0.25123.0). The updated version won't work with boost.
So we should postponed these changes.

RiccardoRossi · 2017-02-23T16:29:09Z

@pooyan-dadvand

i agree. This is a blocker. Writing an issue for the upstream library

loumalouomega · 2018-01-19T18:14:40Z

@RiccardoRossi the branch is still opened

philbucher · 2018-05-18T15:42:00Z

now that boost python has been replaced maybe you want to give this another try?

RiccardoRossi · 2018-05-18T17:48:04Z

I think it would be worth... Riccardo

…

On Fri, May 18, 2018 at 5:42 PM, Philipp Bucher ***@***.***> wrote: now that boost python has been replaced maybe you want to give this another try? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#58 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHr7ETc713Xf72a6x3S6tR4F5PnDfrClks5tzuvIgaJpZM4MHMnJ> .

-- *Riccardo Rossi* PhD, Civil Engineer member of the Kratos Team: www.cimne.com/kratos Tenure Track Lecturer at Universitat Politècnica de Catalunya, BarcelonaTech (UPC) Full Research Professor at International Center for Numerical Methods in Engineering (CIMNE) C/ Gran Capità, s/n, Campus Nord UPC, Ed. B0, Despatx 102 (please deliver post to the porters of building C1) 08034 – Barcelona – Spain – www.cimne.com - T.(+34) 93 401 56 96 skype: *rougered4* <http://www.cimne.com/> <https://www.facebook.com/cimne> <http://blog.cimne.com/> <http://vimeo.com/cimne> <http://www.youtube.com/user/CIMNEvideos> <http://www.linkedin.com/company/cimne> <https://twitter.com/cimne> Les dades personals contingudes en aquest missatge són tractades amb la finalitat de mantenir el contacte professional entre CIMNE i voste. Podra exercir els drets d'accés, rectificació, cancel·lació i oposició, dirigint-se a cimne@cimne.upc.edu. La utilització de la seva adreça de correu electronic per part de CIMNE queda subjecte a les disposicions de la Llei 34/2002, de Serveis de la Societat de la Informació i el Comerç Electronic. Imprimiu aquest missatge, només si és estrictament necessari. <http://www.cimne.com/>

loumalouomega · 2018-05-18T20:04:16Z

Which is the difference respect the current version?

RiccardoRossi · 2018-05-18T20:21:39Z

nlohmann is a much cleaner c++11 based implementation. also it natively uses std containers

…

On Fri, May 18, 2018 at 10:04 PM, Vicente Mataix Ferrándiz < ***@***.***> wrote: Reopened #58 <#58>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#58 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHr7EUx9M6ds1UJyyRBk_zxUX2gpvBNaks5tzylBgaJpZM4MHMnJ> .

-- *Riccardo Rossi* PhD, Civil Engineer member of the Kratos Team: www.cimne.com/kratos Tenure Track Lecturer at Universitat Politècnica de Catalunya, BarcelonaTech (UPC) Full Research Professor at International Center for Numerical Methods in Engineering (CIMNE) C/ Gran Capità, s/n, Campus Nord UPC, Ed. B0, Despatx 102 (please deliver post to the porters of building C1) 08034 – Barcelona – Spain – www.cimne.com - T.(+34) 93 401 56 96 skype: *rougered4* <http://www.cimne.com/> <https://www.facebook.com/cimne> <http://blog.cimne.com/> <http://vimeo.com/cimne> <http://www.youtube.com/user/CIMNEvideos> <http://www.linkedin.com/company/cimne> <https://twitter.com/cimne> Les dades personals contingudes en aquest missatge són tractades amb la finalitat de mantenir el contacte professional entre CIMNE i voste. Podra exercir els drets d'accés, rectificació, cancel·lació i oposició, dirigint-se a cimne@cimne.upc.edu. La utilització de la seva adreça de correu electronic per part de CIMNE queda subjecte a les disposicions de la Llei 34/2002, de Serveis de la Societat de la Informació i el Comerç Electronic. Imprimiu aquest missatge, només si és estrictament necessari. <http://www.cimne.com/>

loumalouomega · 2018-11-17T18:28:17Z

This is ready @RiccardoRossi and @pooyan-dadvand

philbucher · 2018-11-17T19:12:19Z

kratos/sources/kratos_parameters.cpp

@@ -228,6 +228,14 @@ Parameters::Parameters(Parameters const& rOther)
 /***********************************************************************************/
 /***********************************************************************************/

+Parameters::Parameters(Parameters&& rOther) : Parameters()
+{
+    swap(rOther);


if this is custom then maybe a different name would be better to not confuse it with the one in the standard ...?

The current implementation already has a swap method

Kratos/kratos/includes/kratos_parameters.h

Line 467 in dfe73ec

void swap(Parameters& rOther) noexcept

This is on master

pooyan-dadvand · 2018-11-21T08:33:27Z

@philbucher does it work with Intel compiler? We should be very cautious with compiler compatibility of new libraries...

philbucher · 2018-11-27T08:06:54Z

I tested it with Intel and it seems to work 👍
I ran multiple tests in release and fulldebug without problems

philbucher · 2018-12-11T09:39:23Z

is this happening?

then we could also implement the long-awaited #2241

loumalouomega · 2018-12-11T11:05:05Z

I will solve the conflict, but this is ready to merge

loumalouomega · 2018-12-11T11:29:32Z

Conflict solved

philbucher · 2018-12-12T00:29:06Z

kratos/python/add_kratos_parameters_to_python.cpp

 pybind11::list items(Parameters const& self)
 {
    pybind11::list t;
-    for(Parameters::const_iterator it=self.begin(); it!=self.end(); ++it)
+    for(auto it=self.begin(); it!=self.end(); ++it)


does auto determine the correct type here? => iterator or const_iterator?

With iterators I am usually a bit unsure

@roigcarlo do you know?

The self is const, so the auto can be only a const iterator.. In any can be changed

Suggested change

for(auto it=self.begin(); it!=self.end(); ++it)

for(Parameters::const_iterator it=self.begin(); it!=self.end(); ++it)

Reverted @philbucher

philbucher · 2018-12-12T00:31:15Z

kratos/includes/kratos_parameters.h


 // External includes
+#include "json/json_fwd.hpp" // Import forward declaration nlohmann json library


could you explain briefly what exactly you did with this?
I know it is needed to pass Travis, but it would be good to know in case someone wants to update the library

Th forward declaration is only a declaration of the classes (just the name), which means that you can work only with references and pointers, so the compiler can manage the memory for that

https://en.wikipedia.org/wiki/Forward_declaration

philbucher · 2018-12-12T00:32:05Z

external_libraries/json/json_fwd.hpp

@@ -0,0 +1,79 @@
+#ifndef NLOHMANN_JSON_FWD_HPP


is this the original file that you downloaded or did you modify it?

This is the one in the json repository + the declaration of the iterators (in the original just the json class is forward declared)

@philbucher

Asked by @philbucher

philbucher

thx for the effort

I meant that we can also add it in the future, since this is the eternal PR already and now it needs again to be reviewed

If you want you could revert and we can add it in the future

philbucher · 2019-01-03T14:53:57Z

kratos/includes/kratos_parameters.h

+     * This version walks and validates the entire json tree below the point at which the function is called
+     * @param rDefaultParameters Parameters of reference which we use to check
+     */
+    void RecursivelyValidateAndAssignDefaults(const Parameters& rDefaultParameters);


I think this method should only accept a const object, why would it need to modify the input?

I would not change the current behavior right now

pooyan-dadvand · 2019-01-11T09:09:12Z

To be discussed in the next week @KratosMultiphysics/technical-committee meeting.

loumalouomega · 2019-01-11T09:13:00Z

Hurray El vie., 11 ene. 2019 10:09, Pooyan Dadvand <notifications@github.com> escribió:

…

To be discussed in the next week @KratosMultiphysics/technical-committee <https://github.com/orgs/KratosMultiphysics/teams/technical-committee> meeting. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#58 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ATEMgCg8TEgVl4DJ0HGnmSzaAmUSnNlPks5vCFS6gaJpZM4MHMnJ> .

pooyan-dadvand · 2019-01-16T11:43:01Z

Special thanks to @RiccardoRossi and @loumalouomega for their effort on this! 👍

loumalouomega · 2019-01-16T11:43:28Z

I am so happy, finally

loumalouomega · 2019-01-16T11:43:51Z

I am so happy, finally

Finally

RiccardoRossi added 10 commits February 15, 2017 12:31

committing files to replace rapidjson to nlohmann. TO BE DISCUSSED, n…

b643c1f

…ot yet to be merged

committing files to replace rapidjson to nlohmann. TO BE DISCUSSED, n…

c32544c

…ot yet to be merged

Merge branch 'replace-rapidjson-to-nlohmann-json' of https://github.c…

5af2320

…om/KratosMultiphysics/Kratos into replace-rapidjson-to-nlohmann-json

adding a small test and correcting an error.

b00a734

the branch is almost ready for being merged, however an issue with the printing of doubles still remains. Waiting for feedback from the nlohmann library author, who acknowledged the bug

passing strings as const reference and adding better support for prin…

c1178de

…ting in python

merging the master branch into my local branchMerge branch 'master' i…

8be9068

…nto replace-rapidjson-to-nlohmann-json

Merge branch 'master' into replace-rapidjson-to-nlohmann-json

add040e

merging master into local branch prior to committing

ebfa4f1

Merge branch 'master' of https://github.com/KratosMultiphysics/Kratos …

befcfe3

…into replace-rapidjson-to-nlohmann-json

updating nlohmann library to an upgraded version with a bug correction

81d23fc

josep-m-carbonell mentioned this pull request Feb 23, 2017

Develompents for tables and properties for the usage in the ConstitutiveLaw #31

Closed

adding iterators

3d6440f

pooyan-dadvand reviewed Feb 23, 2017

View reviewed changes

RiccardoRossi closed this Feb 23, 2017

roigcarlo deleted the replace-rapidjson-to-nlohmann-json branch May 8, 2017 07:39

roigcarlo restored the replace-rapidjson-to-nlohmann-json branch May 8, 2017 07:39

loumalouomega reopened this May 18, 2018

philbucher reviewed Nov 17, 2018

View reviewed changes

loumalouomega added 3 commits December 11, 2018 12:21

Avoid conflict

e7cc9e8

Merge branch 'master' into replace-rapidjson-to-nlohmann-json

d2939e7

Solve conflict

b13f852

philbucher reviewed Dec 12, 2018

View reviewed changes

Revert Parameters::const_iterator

2bc2303

loumalouomega added Kratos Core External library labels Dec 15, 2018

philbucher mentioned this pull request Dec 30, 2018

[Core][MeshingApplication] Extend gradient process to other geometries. Reduce the number of template arguments to reduce compilation time #3702

Merged

loumalouomega added 2 commits December 30, 2018 16:05

Merge branch 'master' into replace-rapidjson-to-nlohmann-json

4a996ec

Adding const methods

7c55e1f

Asked by @philbucher

philbucher reviewed Jan 3, 2019

View reviewed changes

pooyan-dadvand approved these changes Jan 16, 2019

View reviewed changes

loumalouomega merged commit e2e88f4 into master Jan 16, 2019

loumalouomega deleted the replace-rapidjson-to-nlohmann-json branch January 16, 2019 11:44

philbucher mentioned this pull request Jan 18, 2019

[Core][VTK] make vtk default settings static #3871

Closed

	for(auto it=self.begin(); it!=self.end(); ++it)
	for(Parameters::const_iterator it=self.begin(); it!=self.end(); ++it)


		// External includes
		#include "json/json_fwd.hpp" // Import forward declaration nlohmann json library

Replace rapidjson to nlohmann json #58

Replace rapidjson to nlohmann json #58

Conversation

RiccardoRossi commented Feb 21, 2017

loumalouomega commented Feb 22, 2017

RiccardoRossi commented Feb 22, 2017 via email

pooyan-dadvand commented Feb 22, 2017

josep-m-carbonell commented Feb 22, 2017

RiccardoRossi commented Feb 22, 2017 via email

josep-m-carbonell commented Feb 22, 2017 • edited Loading

RiccardoRossi commented Feb 23, 2017

pooyan-dadvand left a comment

Choose a reason for hiding this comment

RiccardoRossi commented Feb 23, 2017 • edited Loading

loumalouomega commented Jan 19, 2018

philbucher commented May 18, 2018

RiccardoRossi commented May 18, 2018 via email

loumalouomega commented May 18, 2018

RiccardoRossi commented May 18, 2018 via email

loumalouomega commented Nov 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pooyan-dadvand commented Nov 21, 2018

philbucher commented Nov 27, 2018

philbucher commented Dec 11, 2018

loumalouomega commented Dec 11, 2018

loumalouomega commented Dec 11, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

philbucher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pooyan-dadvand commented Jan 11, 2019

loumalouomega commented Jan 11, 2019 via email

pooyan-dadvand commented Jan 16, 2019

loumalouomega commented Jan 16, 2019

loumalouomega commented Jan 16, 2019

josep-m-carbonell commented Feb 22, 2017 •

edited

Loading

RiccardoRossi commented Feb 23, 2017 •

edited

Loading