1912/1 item schema #1913
1912/1 item schema #1913
Conversation
ginkgo-bot
added
reg:build
MarcelKoch
force-pushed
the
1912/1-item-schema
branch
from
8c2f622 to
d038e2b
Compare
| "type": "string", | ||
| "pattern": "^[a-z0-9]+-[a-z0-9]+" | ||
| }, | ||
| "reorder": { |
There was a problem hiding this comment.
if we only have natural reorder, should we provide this option now?
There was a problem hiding this comment.
Since the reorder key is not required, this only checks if the reorder key is provided, then it has to be natural.
MarcelKoch
force-pushed
the
1912/1-item-schema
branch
from
d038e2b to
c2ffa29
Compare
| "if": { | ||
| "required": [ | ||
| "stride" | ||
| ] | ||
| }, |
There was a problem hiding this comment.
could you share the documentation for this?
I interpret it like if required stride then ...
but it never require stride right?
There was a problem hiding this comment.
Maybe this can help: https://json-schema.org/understanding-json-schema/reference/conditionals#ifthenelse.
I think the if tries to check if anything specified here validates. So it checks if the required validates, and based on that chooses the then or else branch.
| ginkgo | ||
| gflags | ||
| nlohmann_json::nlohmann_json | ||
| nlohmann_json_schema_validator |
There was a problem hiding this comment.
also need to add the license from nlohmann_json_schema_validator
| "stencil": { | ||
| "type": "object", | ||
| "properties": { | ||
| "name": { |
There was a problem hiding this comment.
| "name": { | |
| "discretization": { |
There was a problem hiding this comment.
IMO name is shorter and just as descriptive (with the right documentation). I will leave it as is.
There was a problem hiding this comment.
How about kind? I.e. what kind of stencil is it? type conflicts with other categories of types, but kind is not something we have right now.
There was a problem hiding this comment.
That's better, thanks for the suggestion!
| "filename": { | ||
| "type": "string" | ||
| }, | ||
| "stencil": { |
There was a problem hiding this comment.
this will also lead the format changes. Is it intentional?
previously,
"stencil": "5pt",
"local_size" ....
now
"stencil": {
"name": "5pt",
...
}
There was a problem hiding this comment.
Yes, because I find this more consistent. But if others prefer the old version, I can revert.
benchmark/schema/spmv.reuse.json
Outdated
| "properties": { | ||
| "format": { | ||
| "enum": [ | ||
| "coo", |
There was a problem hiding this comment.
Do we also remove the format out of CLI?
some of them might be hard to understand without comment
There was a problem hiding this comment.
It will be removed from the CLI, but there will be extra documentation to make up for it.
There was a problem hiding this comment.
Same argument as for solvers: I think this is an error that can be reasonably caught inside the benchmark code, unless we consider it unlikely that there will be other SpMV implementations we want to test temporarily. Right now, we have two places we need to modify if we add a new matrix format, this would make it three.
MarcelKoch
force-pushed
the
1912/0-identity-config
branch
from
a0d77bd to
02b84db
Compare
MarcelKoch
force-pushed
the
1912/1-item-schema
branch
from
c2ffa29 to
9e06322
Compare
benchmark/CMakeLists.txt
Outdated
| ) | ||
| endif() | ||
|
|
||
| include(FetchContent) |
There was a problem hiding this comment.
FetchContent things should happen in third_party ideally, and this would make building Ginkgo in air-gapped systems difficult. Since this is a library we don't export, it may be reasonable to vendor it, but if we didn't, then we would need to add it to spack etc. if we ever want to make the benchmark executables installable artifacts.
There was a problem hiding this comment.
You're right. This was more of a temporary solution that I forgot about.
| "stencil": { | ||
| "type": "object", | ||
| "properties": { | ||
| "name": { |
There was a problem hiding this comment.
How about kind? I.e. what kind of stencil is it? type conflicts with other categories of types, but kind is not something we have right now.
benchmark/schema/spmv.reuse.json
Outdated
| "properties": { | ||
| "format": { | ||
| "enum": [ | ||
| "coo", |
There was a problem hiding this comment.
Same argument as for solvers: I think this is an error that can be reasonably caught inside the benchmark code, unless we consider it unlikely that there will be other SpMV implementations we want to test temporarily. Right now, we have two places we need to modify if we add a new matrix format, this would make it three.
MarcelKoch
force-pushed
the
1912/1-item-schema
branch
from
9e06322 to
213ff21
Compare
4 participants
This PR adds JSON schema definitions for the new benchmark input.