Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

json scheme #12835

Open
RiccardoRossi opened this issue Nov 8, 2024 · 6 comments
Open

json scheme #12835

RiccardoRossi opened this issue Nov 8, 2024 · 6 comments

Comments

@RiccardoRossi
Copy link
Member

Description
@matekelemen @pooyan-dadvand i am sitting at the airport and i did (with the help of chatgpt) the exercise of generating a json scheme for amgcl, adding the possible choices and the current defaults. Indeed it looks cleaner that i expected.

to be honest i am not 100% sure this schema is valid ... but i think it should be close enough

{
    "$schema": "http://json-schema.org/draft-07/schema#",
    "title": "Solver Configuration",
    "type": "object",
    "properties": {
        "preconditioner_type": {
            "type": "string",
            "default": "amg",
            "enum": ["amg", "relaxation", "dummy"]
        },
        "solver_type": {
            "type": "string",
            "default": "amgcl"
        },
        "smoother_type": {
            "type": "string",
            "default": "ilu0",
            "enum": ["spai0", "spai1", "ilu0", "ilut", "iluk", "damped_jacobi", "gauss_seidel", "chebyshev"]
        },
        "krylov_type": {
            "type": "string",
            "default": "gmres",
            "enum": ["gmres", "bicgstab", "cg", "bicgstabl", "lgmres", "fgmres", "bicgstab_with_gmres_fallback", "idrs"]
        },
        "coarsening_type": {
            "type": "string",
            "default": "aggregation",
            "enum": ["ruge_stuben", "aggregation", "smoothed_aggregation", "smoothed_aggr_emin"]
        },
        "max_iteration": {
            "type": "integer",
            "default": 100,
            "minimum": 1
        },
        "provide_coordinates": {
            "type": "boolean",
            "default": false
        },
        "gmres_krylov_space_dimension": {
            "type": "integer",
            "default": 100,
            "minimum": 1
        },
        "verbosity": {
            "type": "integer",
            "default": 1,
            "minimum": 0,
            "maximum": 3
        },
        "tolerance": {
            "type": "number",
            "default": 1e-6,
            "minimum": 0
        },
        "scaling": {
            "type": "boolean",
            "default": false
        },
        "block_size": {
            "type": "integer",
            "default": 1,
            "minimum": 1
        },
        "use_block_matrices_if_possible": {
            "type": "boolean",
            "default": true
        },
        "coarse_enough": {
            "type": "integer",
            "default": 1000,
            "minimum": 1
        },
        "max_levels": {
            "type": "integer",
            "default": -1,
            "minimum": -1
        },
        "pre_sweeps": {
            "type": "integer",
            "default": 1,
            "minimum": 0
        },
        "post_sweeps": {
            "type": "integer",
            "default": 1,
            "minimum": 0
        },
        "use_gpgpu": {
            "type": "boolean",
            "default": false
        }
    },
    "required": ["solver_type"],
    "additionalProperties": false
}
@loumalouomega
Copy link
Member

Instead of Chatgpt I would use an API key to be able to do this in a more systematic manner. For some reason LLM are not good with JSON, see https://github.com/imaurer/awesome-llm-json

@RiccardoRossi
Copy link
Member Author

RiccardoRossi commented Nov 8, 2024

Well, the point here was to do the exercise of underdtanding how a schema woukd look like for a parameter of medium complexity like the one of amgcl

Do you think that the output wpuld look different domingo it in a different way?

@loumalouomega
Copy link
Member

Well, the point here was to do the exercise of underdtanding how a schema woukd look like for a parameter of medium complexity like the one of amgcl

Do you think that the output wpuld look different domingo it in a different way?

Not, it would look similar. I am just saying we have many many classes set via Parameters. So the ideal would be to do it in the most systematic manner, as it is not a complex task, but repetitive and tiresome.

@RiccardoRossi
Copy link
Member Author

well, first of all we need to decide if this is the way we want to pursue.
(this is a poposal by @matekelemen), i expressed my concern about the verbosity, but it actually looks better than what i expected

As i understand the idea of @matekelemen and @pooyan-dadvand would be to have GetDefaults to eventually return a schema to validate against.

@matekelemen
Copy link
Contributor

matekelemen commented Nov 8, 2024

GetDefaults to eventually return a schema to validate against

Yes, this would eventually be one of the goals. It would have some other advantages as well:

  • Any object receiving its configuration from JSON files or Parameters could outsource its input validation to the schema.
  • Default settings can be defined in the schema.
  • Options for a specific object's configuration could be interactively queried, using the schema. This means that
    • we could have hinting for JSON configs right inside the IDE
    • FlowGraph nodes could be automatically generated
  • nlohmann_json (the third-party JSON lib we use in C++) supports schemas, so we don't have to add any additional dependencies.
  • Adoption can happen gradually. New classes can be written with schema support while older ones can be ported later.

The obvious trade-offs are that

  • Schemas are relatively verbose, as @RiccardoRossi 's example demonstrates, and are quite a bit of extra effort to write.
  • Changing a class' config interface would also have to be reflected in their schemas.

I must confess that I've never used JSON schemas before, and I've only heard about them from @Igarizza. Still, I think they have enough to offer to be worth the hassle. I say let's at least give them a shot.

@pooyan-dadvand
Copy link
Member

Just to add that I agree with @matekelemen and I think we should tackle this gradually. Also I agree with @RiccardoRossi that we should first define what we want to put in the GetSpecification method before start populating them everywhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants