NeuroLang / NeuroLang

Neurolang enables the analysis of NeuroImaging data through probabilistic logic programming. It seamlessly allow to combine, images, databases, and ontologies within a single framework.
BSD 3-Clause "New" or "Revised" License
6 stars 11 forks source link

Extended projection operator implementation appends columns and should only project those specified #296

Closed demianw closed 4 years ago

demianw commented 4 years ago

The current implementation of the extended projection operation as it's currently implemented, appends the projected columns to the RA set. This is not in agreement with the theory and should be changed for consistency. Same with the Relational Algebra solvers (provenance and traditional)

See https://github.com/NeuroLang/NeuroLang/blob/5bc896d418d93e0e51a6e752a34fb77611a654b1/neurolang/utils/tests/test_relational_algebra_set.py#L346

tgy commented 4 years ago

What about having an ExtendedAggregation and using a normal Projection to get the same result you are mentioning? I'm thinking this would be more flexible because sometimes we might want to create a new column without removing the others.

demianw commented 4 years ago

We could have one, but Extended Aggregation is not the case as aggregation in DB theory means computation on rows into a new row. Issue is always how moving away from the theory makes algorithms harder to understand in perspective (Because you need to know theory + theory extensions). But if you find a good name, I'm up for it. In any case, the "comfort" is just adding the list of current columns to the projection, is not that hard of an issue.

tgy commented 4 years ago

Yes, that makes sense to stay close to the theory! I would be happy to fix this issue, having implemented the ExtendedProjection and knowing the theory behind it.

demianw commented 4 years ago

As you want. @gzanitti reimplented the back-end and some of his algorithms might depend on it. So it's better if you coordinate on the #neuroinformatics_core channel

tgy commented 4 years ago

this is fixed in #308