Skip to content

Conversation

greole
Copy link
Collaborator

@greole greole commented Nov 5, 2024

This PR adds a function to create gko::config::pnode directly from a std::string.

Motivation: We would like to use Ginkgo's file config solver within the pyGinkgo bindings. The current implementation of the config solver seems to rely on json files. However, creating temporary files is a nuisance which can cause all kinds issues and is not really necessary in our case. Thus, just passing a std::string to generate a property tree would simplify things a lot.

@greole greole requested a review from yhmtsai November 5, 2024 08:21
@ginkgo-bot ginkgo-bot added the mod:core This is related to the core module. label Nov 5, 2024
Copy link
Member

@upsj upsj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I thought we had this already

/**
* parse_json_string takes a json string to generate the property tree object
*/
inline gko::config::pnode parse_json_string(std::string json)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it's a good time to start introducing std::string_view to the code base, now that we support C++17

Copy link
Member

@yhmtsai yhmtsai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to add a test for it in extensions/test/json_config
question for pyGinkgo:
do you have any intermediate layer in C or C++ between ginkgo and pyGinkgo?

@greole
Copy link
Collaborator Author

greole commented Nov 5, 2024

need to add a test for it in extensions/test/json_config

sure i'll add one

question for pyGinkgo:
do you have any intermediate layer in C or C++ between ginkgo and pyGinkgo?

we use pybind11 , which is c++, to generate any intermediate code to register to python.

@greole greole force-pushed the feat/parse_json_string branch from 09023e0 to 62d4f08 Compare December 5, 2024 08:23
@greole greole requested a review from yhmtsai December 5, 2024 08:57
@yhmtsai yhmtsai added 1:ST:ready-to-merge This PR is ready to merge. 1:ST:skip-full-test 1:ST:no-changelog-entry Skip the wiki check for changelog update labels Dec 5, 2024
@greole greole merged commit 3d30c04 into develop Dec 5, 2024
9 of 11 checks passed
@greole greole deleted the feat/parse_json_string branch December 5, 2024 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1:ST:no-changelog-entry Skip the wiki check for changelog update 1:ST:ready-to-merge This PR is ready to merge. 1:ST:skip-full-test mod:core This is related to the core module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants