-
Notifications
You must be signed in to change notification settings - Fork 232
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
Sorting and disabling functionality for ExplanationServices #1059
Sorting and disabling functionality for ExplanationServices #1059
Conversation
@stefborg I am inclined to merge this for the upcoming 5.6.4 release, are you still happy with that PR as it is? |
I can try to review this today as similar PR is open for the protege-proof-explanation plugin, which also needs to be updated. |
I just checked the PR. The work looks solid! Many thanks @stefborg! I found just a small glitch: when resetting preferences, the list of installed explanation services becomes empty when preferences are opened again. Only when preferences are opened for the second time the list is filled with installed plugins. As a side note: there is also a similar InconsistentOntologyManager, which manages InconsistentOntologyPlugins (yes, these are different from ExplanationPlugins!). However, there is no UI to similarly manage these plugins. Not sure if it is a good idea to add similar settings. |
Thank you, @ykazakov for testing the code! I will have a look today at the issue you mentioned. Regarding the InconsistentOntologyManager, I didn't notice it so far because we don't often get inconsistent ontologies. But you are right, in principle it should get the same preferences, which would require some duplicate work (probably also for the proof-based inconsistency explanations). It would be simpler if the inconsistency explanation services could somehow "inherit" the settings from the explanation services (at least for those services that are using the same underlying algorithm). But that would require more work, for example merging ExplanationService and InconsistentOntologyPluginInstance, and allowing them to declare whether they support explaining inconsistent ontologies, OWLAxioms, or both. |
I have fixed the issue by reloading the ExplanationManager whenever the preference dialog is disposed, instead of just on a successful "applyChanges" (i.e. after clicking "OK"). I also encountered a strange behavior in Protégé, which I am not sure is intended: When the preferences are reset, it only deletes the preferences node, but does not close the preferences window. When clicking "OK", it does not actually save the (currently still visible) previous preferences again, but the preferences remain deleted. After resetting, I would have expected the preference dialog to be either closed or reloaded to show the new (reset) preference values. |
Thanks @stefborg, that was fast! I will take a look at your changes.
I think that would be hard to implement since, in principle, these two extension points (for explanation and inconsistency explanation) are unrelated: they can be implemented by different plugins, or by the same plugin but in a different ways, and even by the same plugin many times. What about simply listing both explanation plugins and inconsistency explanation plugins in the same table, and allow reorder them in any way? Of course, only applicable plugins should be actually used, but this already applies to explanation plugins.
I think this is related to #168 I also gave another thought about the UI: the two options for "Default explanation service" seem exclusive, but in fact the "first applicable explanation service" is also used when "the most recently used explanation service" is not applicable to the selected entailment.
This should also take less horizontal space since everything is in one column. |
I forgot that this "two column design" is used for all preferences, so 1. should probably be left as a label for the table 2 on the left. |
I think that's a good idea and implemented the change. However, now I wonder whether the functionality that "Protege will try the services in the specified order and will use the first one that can provide an explanation" is clear enough from the UI. This is somehow implicit in the order in the table (and also mentioned in the tooltips), but I am not sure whether a new user would directly understand what the ordering means. What do you think? |
Thank you both for your reactivity, especially on a 2-years old issue! Much appreciated.
For what is worth, this seems clear enough to me, though you can never tell what users will effectively understand. But as long as you’re satisfied with the way the feature works, I am happy to release 5.6.4 with it. We can always tweak the UI later if we hear that users are confused about the way the option is presented. |
Hi, I have now tested the changes. All works well, except for one glitch: the "Move up" and "Move down" buttons are enabled also if there is nothing selected in the table. If pressed, an exception will be thrown. Regarding the clarity of UI, I think the concern of @stefborg is valid, but I did not find an easy solution. Perhaps one can add a label "Change priority" left to "Move up" and "Move down" buttons so that it is at least clear that the order of the plugins in the list matters. Perhaps, also changing / shortening the tooltip for the table could help, e.g., "The first selected plugin that can provide an explanation for an axiom will be used." Even better would be to provide separate tooltips for the columns (e.g., "Priority" for the fist column) but I did not manage to create such tooltips using this tutorial to work. |
Looks like I found another glitch:
|
|
||
@Override | ||
public void applyChanges() { | ||
ExplanationPreferences prefs = ExplanationPreferences.create(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create()
will erase defaultExplanationService
, which is not stored in this preferences. One should probably use preferences that have been loaded during initialize()
JButton buttonDown = new JButton("↓ Move down︎"); | ||
buttonDown.setToolTipText("Move the selected explanation service towards the bottom of the list"); | ||
buttonDown.addActionListener(e -> { | ||
int rowIndex = pluginTable.getSelectedRow(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the panel is created, no row in the table is selected. So either select, e.g., the first row (if there is) or make the buttons not active.
This pull request modifies the general Explanation preferences. Users can choose between (a) always using the most recently used explanation service when requesting a new explanation (current behavior and default) and (b) using the explanation services according to a user-defined order. Additionally, individual explanations services can be disabled in the preferences. The reason is that a user may always want to compute justifications first (because it may be faster), and only request more expensive explanations when the justifications aren't enough.
This came out of a discussion on liveontologies/protege-proof-explanation#2, where this functionality would be useful for sorting and filtering many different proof services (e.g., supplied by https://github.com/liveontologies/elk-reasoner or https://github.com/de-tu-dresden-inf-lat/evee). It makes sense if this functionality is not only implemented at the level of proof services, but also in general for all explanation services. The proof service preferences can then also use the configuration options (a/b) described above.
For now, I tested this with the two explanation services https://github.com/protegeproject/explanation-workbench and https://github.com/liveontologies/protege-proof-explanation, but a similar implementation was tested for 10+ proof services.