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

unify - empty unifier list #120

Open
MaxOstrowski opened this issue Oct 12, 2022 · 2 comments
Open

unify - empty unifier list #120

MaxOstrowski opened this issue Oct 12, 2022 · 2 comments

Comments

@MaxOstrowski
Copy link
Member

Calling unify with an empty Predicate list or None causes the following error:
ValueError: The unifier must be a list of predicates or a SymbolPredicateUnifier
Would it make sense to allow the empty list (or None) and return an empty FactBase ?
In my case, the list is given from outside (maybe user input, maybe problem dependent, and this would make it more general, like a default case.
Do you think this makes sense ?

@daveraja
Copy link
Collaborator

daveraja commented Oct 13, 2022

It would be easy to change but I'm not sure if it is a good idea.

The unification process seems to be a common place for things to go wrong. For example, cases where the user specifies the wrong field in their predicate definition so the facts that they expect to be unified aren't. In general I tend to feel that it makes clorm easier to use if it can detect and report as many mistakes as possible (... and of course, improving the error messages would also help :)) .

With this in mind, I can't think of a case where passing None is not a mistake. However, I'm less certain about the empty list. In most use cases that I can think of it seems to be an obvious error and it would help the user of the library if the function threw an exception. But maybe in your case it might sense. If so maybe we could add a flag to control if an exception is thrown or not?

In your case, if the list of unifiers is given externally does it actually make sense for a user of your system to pass an empty list? Wouldn't it also be a red-flag that the user has done something wrong (in which case catching this exception would help you to report back to the user)?

@MaxOstrowski
Copy link
Member Author

I agree and I do not have a strong opinion about it. In my case I just needed some default value to start coding with and see if everything works. This is not the best use case. There is no actual user interaction, this was a fictional scenario. I could imagine that sometimes you might be interested in some atoms of the solutions and sometimes only in SAT or the optimality value. In this way this could be done with the same code without any 'if'. If this is a good idea in terms of error detection I actually don't know. Maybe someone else has an oppinion about it or there are similar functions in other ORM libraries ... Sorry

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