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

Remove and filter operations new "drop_axiom_annotation" option #1023

Merged
merged 8 commits into from
Feb 7, 2024
Merged

Remove and filter operations new "drop_axiom_annotation" option #1023

merged 8 commits into from
Feb 7, 2024

Conversation

hkir-dev
Copy link
Contributor

@hkir-dev hkir-dev commented Jul 6, 2022

Resolves [#886]

  • docs/ have been added/updated
  • tests have been added/updated
  • mvn verify says all tests pass
  • mvn site says all JavaDocs correct
  • CHANGELOG.md has been updated

New parameter drop_axiom_annotation added to remove and filter operations. An annotation property is provided to this option to drop/remove all axiom annotations using the property.

@hkir-dev hkir-dev requested a review from matentzn July 6, 2022 12:32
Copy link
Contributor

@matentzn matentzn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent design to allow specifying the APs to remove.

One thing that would be great: can we add a blanket option for --drop-axiom-annotations (should be camel case, and should have an s in the end, because there can be multiple), that allows removing all axioms annotations? Like --drop-axiom-annotations all?

CHANGELOG.md Outdated Show resolved Hide resolved
docs/filter.md Outdated Show resolved Hide resolved
docs/filter.md Outdated Show resolved Hide resolved
docs/remove.md Outdated Show resolved Hide resolved
docs/remove.md Outdated Show resolved Hide resolved
@hkir-dev hkir-dev requested a review from matentzn February 20, 2023 19:13
Copy link
Contributor

@matentzn matentzn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks great, let me summarise its current behaviour.

  1. We introduce a new option --drop-axiom-annotations which removes all axiom annotations (or those using one or more specific properties) from a target set of axioms, determined by either the filter or remove commands.
  2. It works identically in remove and filter. This is cognitively much easier than applying "filter" logic in "filter" (i.e. only including the selected axiom annotations in the target set), which is correctly reserved to the --select/--axioms pipeline.

The only slight question mark I have, without wanting to cause an issue, is whether

  1. This should be a command in its own right after all robot remove-axiom-annotations --select all. Basically, we could then pipe the output of filter/remove to remove-axiom-annotations to achieve the same effect, without overloading the remove command too much.
  2. Or, even if we go with the current implementation, we may not need to extend filter, and instead rely on passing the output of filter to remove --drop-axiom-annotations all.

I will let @jamesaoverton think about this one, but I think this is the last remaining question here.

@jamesaoverton
Copy link
Member

@matentzn I really dropped the ball on this one. Let's talk about it again in our next meeting, or sooner if urgent.

@jamesaoverton
Copy link
Member

@matentzn We never remembered to discuss this in our meetings. What's the state of play here?

@matentzn
Copy link
Contributor

The implementation for this PR is correct, and we can use it to do what we want. But I have some doubts about the cleanliness of the design, as described in my comment above. To get in the headspace again:

Remove/filter does its thing to the very end.... and then, the target set (aka whatever remains) gets post processed using the --drop-axiom-annotations option. This does overload the filter and axiom commands a little bit, as this is, indeed, a conceptually independent operation. On the other hand: practically, I cant think of a case where we would use this command independently of filter and remove (maybe with a tiny exception of "extract", but very corner case), and there is overhead in adding a new command - you are a better judge of the trade-offs here.

@jamesaoverton
Copy link
Member

I realized that the integration tests were not running, so I made #1181 to fix that.

@jamesaoverton jamesaoverton merged commit c480381 into ontodev:master Feb 7, 2024
3 checks passed
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

Successfully merging this pull request may close these issues.

3 participants