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

No current way to get correct expressions #1172

Closed
ampli opened this issue Mar 22, 2021 · 4 comments
Closed

No current way to get correct expressions #1172

ampli opened this issue Mar 22, 2021 · 4 comments
Assignees

Comments

@ampli
Copy link
Member

ampli commented Mar 22, 2021

Exp already is in the public

Note that they may contain dialect components and there is no public API to convert them to fixed costs.
Something like this: lg_exp_set_dialect(Exp *e, Parse_Options opts).

Originally posted by @ampli in #1146 (reply in thread)

@ampli
Copy link
Member Author

ampli commented Mar 22, 2021

I would like to send a PR for that, because else expressions from dictionary_lookup_list() and dictionary_get_category()
are not useful. (Providing dialect-resolved expressions in them is not a good option.)

Is the following fine?

Exp *exp_with_dialect_resolved = lg_exp_set_dialect(const Exp *e, const Parse_Options opts);
void free_Exp(Exp *e);

@linas
Copy link
Member

linas commented Mar 22, 2021

Is the following fine?

I guess. Right now, the only thing that is using the Exp API is the opencog/nlp/lg-dict code, that imports expressions into the atomspace. These are used in the microplanner/sureal generation code, which is currently unmaintained. So there is no urgency to fix this.

@ampli ampli self-assigned this Mar 22, 2021
@ampli ampli mentioned this issue Mar 23, 2021
@ampli
Copy link
Member Author

ampli commented May 1, 2021

There is another naming option:

Exp *exp_with_dialect_resolved = lg_exp_copy(const Exp *e, Parse_Options opts);

The idea here is that now we need to set only the dialect, but we can leave it open for setting other things according to new parse-options (that just now maybe we cannot even guess).

For freeing the expression, I guess lg_exp_free() is a more consistent naming. (Or maybe it is redundant, as lg_exp_copy() can allocate the expression in contiguous memory so just free can be used. But using a special function to free expression memory may be more flexible.)

Comments:

  1. Setting the dialect is currently "destructive" (due to efficiency considerations), which means you cannot set it again in the same expression. So for now if a program wants to tune it (to try a more permissive setting etc.) it needs to keep the original expression. It can be solved if needed (in two ways at least).
  2. The dictionary expressions may have a complex internal construction due to the way they are written (arbitrary parens, optional connectors, costs, dialects, macro references). I wrote an expression simplifier that bring them to a "canonical" form that is easier to process. The idea was to use it before building the disjuncts (because it is then done much faster). It can also be used in this lg_exp_copy().

@linas
Copy link
Member

linas commented May 3, 2021

I think that lg_exp_resolve() would be a better name: it is not "just a copy", its a different expression, one with "ambiguities resolved".

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

2 participants