Property tree

[yhmtsai]

This is the property tree again because I can not reopen the old pull request #1295
Note. we consider the internal structure again because it might be easy as a bridge when we port to another language.

• pnode -> ptree? or name suggestion
• parser place. The parser is quite short in the header, so we can keep them in ginkgo public header folder, but don't include them in ginkgo.hpp. Users include them if they need. move to another pr
• parser can be another pr.

[upsj]

Looks really good already, I just had a few nits to simplify the setup and shrink the headers.

[MarcelKoch]

I think this (and the other at functions) should allow returning an empty pnode, if it doesn't contain the object. This, together with either bool empty() or a bool conversion operator, can allow easier code:

if(auto child = pt.at("name"){
  parse(child);
}

instead of repeating pt.contains(...) and pt.at(...).

[yhmtsai]

I add bool() and get additionally.
I would like to keep the same behavior between const and non-const at.
Thus, I add additional get const version to do that.

if(auto child = pt.at("name"){
// if pt.at("name") is not empty or name is not existed.
  parse(child);
}

I do not distinguish whether the empty is from user or it is not existed.

[MarcelKoch]

TBH. I've not commented on that yet, but I would prefer only const access.

[MarcelKoch]

Perhaps instead of get, adding a default return value to at, in case the string is not found, would be better. get and at do nearly the same thing, so it might be confusing which one to use.

[yhmtsai]

currently, I return pnode& from at() and const pnode& from at() const. (For less copy in auto& temp = pnode.at())
Return the empty node in at() is weird because it should not be modified and the modification does not change the content of property tree.
If only make it available in at() const, we need to cast it to const first for const at. However, it also introduces different non-found behavior.

[MarcelKoch]

Just some first thoughts.

[MarcelKoch]

I think these should actually throw. I don't think we need to care about performance for our configuration.

[MarcelKoch]

ping

[MarcelKoch]

Again, we should throw.

[MarcelKoch]

Do we need mutable accessors? I would really prefer it to be const.

[MarcelKoch]

Perhaps instead of get, adding a default return value to at, in case the string is not found, would be better. get and at do nearly the same thing, so it might be confusing which one to use.

[MarcelKoch]

Regarding the placement of the files, I would suggest putting the headers into ginkgo/extension/.... I think that should make these functions easier available for users.

[yhmtsai]

I have made them install into the ginkgo/extensions.
I am thinking of making the extension self-contained (create folder structure ginkgo/extensions in /extensions/include)or reusing the ginkgo structure (putting the headers in /include/ginkgo/extensions/ directly.)

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
25 Code Smells

92.4% Coverage
0.0% Duplication

The version of Java (11.0.3) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

[yhmtsai]

rebase!

[ginkgo-bot]

Error: Rebase failed, see the related Action for details

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
25 Code Smells

92.4% Coverage
0.0% Duplication

The version of Java (11.0.3) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

[github-actions]

Error: The following files need to be formatted:

core/config/property_tree.cpp
core/test/config/data.cpp
core/test/config/property_tree.cpp
core/test/config/utils.hpp
extension/property_tree/include/property_tree/json_parser.hpp
extension/property_tree/test/json_parser.cpp
include/ginkgo/core/config/data.hpp
include/ginkgo/core/config/property_tree.hpp

You can find a formatting patch under Artifacts here or run format! if you have write access to Ginkgo

[upsj]

I think this could use a bit of a clean-up, but functionality-wise it looks good.

• Headers should not contain implementations
• data and property_tree can be merged
• We need a lot of boiler-plate to make extension work for a single file, I think we can do that with an optionally compiled source file instead, like some of our loggers and MPI.

[MarcelKoch]

Since c++17 comes up here, maybe this is a good point to start considering enabling optional components that require c++17. We are right after a release, so users will have plenty of time to adapt if we use c++17 in the next release. This would go hand-in-hand with raising the compiler version requirements, which we also discussed a bit.

I'm strongly in favor of allowing components to use c++. I would suggest making the use of c++17 only optional, i.e. disabling those components if c++17 is not available.

[yhmtsai]

@upsj For the extension, the idea is that user can use it without recompile ginkgo itself. when they need it, include it with json header and ready to go. I agree that it requires more cmake setup to make it work, but for me it more fit the purposes (plug it only when need)
For the data structure, does flatten design give benefit? the current design shows the structure(map, array, data) and underlying data additionally.
for implmentation in the header, it is the template function in theory, so it needs to be in header.
I know it can be replaced by one template + the additional specialization/overload. does it still help reducing the number of symbols because there's still a default template in the header? I am not familar with the symbols. Doesn't inline function expand directly in the code without symbol?
@MarcelKoch Although C++17 is powerful, we still need to somehow provide C++14 interface at least. Some applications using it prefer staying C++14 especially when main ginkgo still uses C++14. I do not know whether we can push them to update version. They use resource manager for two years I think.

[MarcelKoch]

I try to summarize some of my key points here:

pnode interface:

• only const access should be provided
• remove get for array and map, only keep at (possibly with default value)
• no get_data, get_map, get_array, maybe add a size method if necessary

In general I think we can model our interface a lot like the nlohmann interface.

data type:

• doesn't need to be part of the public interface
• compatibility with STL (i.e. config::get, config::holds_alternative) are not necessary

I agree with Tobias, that the json_parser.hpp can be part of our public headers. Also, by not relying on the nlohmann type, we can remove the link time dependency and use the normal pre-processor approach.

[MarcelKoch]

Since the CLI parser was removed, I think there is no need for mutable access anymore.

[upsj]

It might still be necessary later on if we want to allow building configs from existing objects.

[MarcelKoch]

That goes quite a bit further than the scope that is discussed here. And even then I don't think it would be strictly necessary.

[upsj]

Yes, it would probably be sufficient to provide an vector<pnode> constructor. Just wanted to mention it, because I think it will be useful for people to quickly move to the config model from the factory model.

[MarcelKoch]

I think there should only be the at function. Having both at and get at the same time is confusing to me. I would that the at doesn't throw, but instead returns the empty_node if the name is not found. And of course, there should only be the const version of at

[yhmtsai]

at to me is trying to access the existing data. If there is no data, should throw not return a fake object.
get can return some empty node.
I need to leave the non-const access such that parser can create property tree.

[MarcelKoch]

My point is to have only one of them. But I think you are right, maybe it's better to keep only get.
We can use std::vector and/or std::map to create the property tree, no need to keep mutable access.

[MarcelKoch]

Is this needed? We also have the get_data<T> version, which IMO is more useful.

[MarcelKoch]

Also, if we remove this function, then we don't need to expose data publicly. This would allow us to switch to std::variant more easily in the future.

[MarcelKoch]

final

[MarcelKoch]

Suggested change

	inline void json_parser(gko::config::pnode& ptree, const nlohmann::json& dom)
	inline void parse_json(gko::config::pnode& ptree, const nlohmann::json& dom)

to make the function name a verb

[MarcelKoch]

are the get_data, get_array, get_map accessors necessary?
IMO, we should mainly access the data via the get<T> or get(key|i) methods.
The only additional information, the get_* method have is that the size of the array/map is also available. So maybe a size method would be enough.

[MarcelKoch]

I think the get function needs to support more types.
Otherwise we would have to do things like

auto i = static_cast<int32>(config::get<long long int>(data));

instead of just config::get<int32>(...).

[yhmtsai]

it is to provide the same rule as std::variant.
I did the get with different type in the config helper.

[MarcelKoch]

As I mentioned before, I don't think we should aim for std::variant compatability here. This type should not be exposed to the user, so we can easily replace it later on.

[upsj]

An alternative would be having get_string, get_int, get_double functions next to get_array, that would also get rid of the need to use templates. I don't think we need these in a generic context anyways, a solver would always query specific properties by type and name.

[MarcelKoch]

I think that is a good option. But I wouldn't use int or double in the name, since at least int is just wrong. So perhaps we could have get_integer and get_real instead. Then it would be a bit clearer that additional static_cast are necessary.

[yhmtsai]

For me, the template already did the same thing. You get what you require if it is legal. Query specific properties without the template may make the following pr a bit messy. I can try these first to see what changes necessary in the following pr.

[MarcelKoch]

ping

[MarcelKoch]

The option is not documented in our cmake log.

[upsj]

There are still a handful of comments unaddressed, and the implementation exposes a few things to the user that can stay hidden.

[upsj]

An alternative would be having get_string, get_int, get_double functions next to get_array, that would also get rid of the need to use templates. I don't think we need these in a generic context anyways, a solver would always query specific properties by type and name.

[upsj]

It might still be necessary later on if we want to allow building configs from existing objects.

[yhmtsai]

Because we go for get_integer, get_real, ..., I will just copy paste these functions from data to property_tree which is not good for me (no reduced code in property tree by having data). Thus, I change my mind to the flatten design. I also only allow const access now. note. only get(key) will return empty node when key is not there. get(i) will still give out of bound. I also move the parser to another branch.

[MarcelKoch]

LGTM, all my previous comments were addressed. I left only some smaller nits.

TBH, I really like the clean header file. For me this seems like the right direction to go.

[MarcelKoch]

I know I brougt the empty node as default return value up, but I could also imagine to have to versions, one with a single argument which throws for invalid access, and a two argument version with a default value.

For me it's fine as it is, just in case others want to change this,

[yhmtsai]

I think it can be one with bool, like this?

auto get(key, bool throw_error = false) {
  // not-found
  if (throw_error) {throw;}
  else {return empty;}
}

[MarcelKoch]

I'm not sure how much value this would add. A two parameter version would allow us to use default values for our factories quite easily.

[yhmtsai]

like this?

factory.with_reduction_factor(get_value<type>(config.get("reduction_factor", factory.reduction_factor)));

I am not sure. it may be useful in integer or real value, but other complex or pointer will be a bit long code

[MarcelKoch]

Yes, but I guess it's not important.

[upsj]

I only have minor comments related to explicit-ness of functions and documentation, but it looks really good in general. I'll approve once these suggestions were addressed or discussed.

[upsj]

It would probably be safer to make all of these single-argument constructors explicit

Suggested change

	pnode(bool boolean);
	explicit pnode(bool boolean);

[upsj]

see #650 for an in-depth discussion of explicit vs. implicit conversion to bool

Suggested change

	operator bool() const noexcept;
	explicit operator bool() const noexcept;

[upsj]

still needs to be adressed

[yhmtsai]

ah, sorry. I miss this one. will do it

[upsj]

I would reword this slightly, same for the other access functions

Suggested change

	* Get the array. It will throw error if the current node does not hold an
	* Access the array stored in this property node, throws `gko::ExceptionName` if the property node does not store an array.

[upsj]

Alternative wording could be retrieve, I just feel like get the array doesn't properly that the object is a sum type and doesn't always contain an array.

[yhmtsai]

I think get maybe okay which is similar to variant get

[upsj]

for consistency

Suggested change

	std::string get_string() const;
	const std::string& get_string() const;

[yhmtsai]

I think it is cheap here, so I put this like the other bool, double, int.
Or, do you think they all should be returned const reference?

[upsj]

It's both const-correctness semantics and performance - the latter shouldn't matter though. For semantics, consider https://godbolt.org/z/71aKxGG39

#include <string>

std::string foo();

const std::string& bar();

void baz() {
    foo().clear();
    bar().clear();
}

because the second interface returns a const reference, it doesn't allow modifications, so it prevents users doing non-sensical things. Just a very minor thing though, so I don't want to hold this up.

[upsj]

This one has not been addressed yet

[yhmtsai]

I think I prefer just return it as value as the other integer/bool now. If that user changes the value from the return does not change the property tree itself, it is what user wants to do in my mind.

[upsj]

it is what user wants to do in my mind

This operation does not make any sense though, since it creates a temporary copy, clears it and then does not give the user any way to access it. I can see users making the assumption that you can modify the object that way and being surprised - a compiler error is a clear message that it's not possible.
The same is also not possible with the return values from the primitive value access functions (e.g. get_integer()++ fails to compile).

[yhmtsai]

That makes sense.
I originally thought the case auto a = get_string();. I thought it will be const std::string&, which is inconsistent with the others.
However, type of a is actually std::string unless using auto& a = get_string().
will change it

addressed

[upsj]

LGTM except for the return type/explicit bool discussions, but I'll approve already