-
Notifications
You must be signed in to change notification settings - Fork 2
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
Filter for RecordDataBase #159
Conversation
Docs can be previewed here: https://formal-methods-mpi.github.io/Taxonomy.jl/previews/PR159 |
Docs can be previewed here: https://formal-methods-mpi.github.io/Taxonomy.jl/previews/PR159 |
Docs can be previewed here: https://formal-methods-mpi.github.io/Taxonomy.jl/previews/PR159 |
Docs can be previewed here: https://formal-methods-mpi.github.io/Taxonomy.jl/previews/PR159 |
Docs can be previewed here: https://formal-methods-mpi.github.io/Taxonomy.jl/previews/PR159 |
""" | ||
function Base.filter(f, db::RecordDatabase, level::AbstractString)::RecordDatabase | ||
|
||
@assert level in ["Record", "Study", "Model", "Taxon"] "Invalid level. Choose from 'Record', 'Study', 'Model', 'Taxon'" |
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.
Not that happy with this solution, works for now but do you have a more elegant way?
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.
This is fine. I assume that you dislike, that this implementation is not generic in terms of levels, and that you are traversing each layer manually? In such cases, recursion is usually a more elegant way but since the depth is predefined to not exceed 4 this is fine.
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.
@aaronpeikert Yes, recursion was one concern. Going along with that I was also thinking of some way to add the aimed level into the function f
instead having to define it in an extra argument, because that would be more flexible in case new levels are added. However I agree that for now this approach works just fine, so I will leave it at that.
Docs can be previewed here: https://formal-methods-mpi.github.io/Taxonomy.jl/previews/PR159 |
Docs can be previewed here: https://formal-methods-mpi.github.io/Taxonomy.jl/previews/PR159 |
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.
This is a much needed feature. Thank you for taking it on! I love that you invested so much in testing.
""" | ||
function Base.filter(f, db::RecordDatabase, level::AbstractString)::RecordDatabase | ||
|
||
@assert level in ["Record", "Study", "Model", "Taxon"] "Invalid level. Choose from 'Record', 'Study', 'Model', 'Taxon'" |
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.
This is fine. I assume that you dislike, that this implementation is not generic in terms of levels, and that you are traversing each layer manually? In such cases, recursion is usually a more elegant way but since the depth is predefined to not exceed 4 this is fine.
Ignore the failure on nightly. |
Implemented a filter function for databases.
Wrote a method for
Base.filter()
that takes a filter function, a database and, additionally, the level on which the filter function should be applied (Record, Study, Model, Taxon).The filter function can also work with
rating()
orcertainty()
so the function definition works a bit more naturally, see documentation offilter()
.