BlueBrain / nexus-forge

Building and Using Knowledge Graphs made easy
https://nexus-forge.readthedocs.io
GNU Lesser General Public License v3.0
38 stars 19 forks source link

Add `add_image` method to `Dataset` entity #389

Closed crisely09 closed 4 months ago

crisely09 commented 7 months ago

To tackle #388

This is the simplest implementation to "attach" and image, and then use it to add it to any resource, similar to attach for distributions.

Example of how it is used can be found inside the 03 - Storing notebook. The way to add the image would be :

image = forge.attach_image(path, content_type, about)
resource.image = image
codecov-commenter commented 7 months ago

Codecov Report

Attention: Patch coverage is 37.50000% with 5 lines in your changes are missing coverage. Please review.

Project coverage is 75.65%. Comparing base (be27bb6) to head (a391825).

Files Patch % Lines
kgforge/core/archetypes/store.py 25.00% 3 Missing :warning:
kgforge/specializations/resources/datasets.py 50.00% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #389 +/- ## ========================================== - Coverage 75.69% 75.65% -0.05% ========================================== Files 103 103 Lines 6753 6761 +8 ========================================== + Hits 5112 5115 +3 - Misses 1641 1646 +5 ``` | [Flag](https://app.codecov.io/gh/BlueBrain/nexus-forge/pull/389/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlueBrain) | Coverage Δ | | |---|---|---| | [unittests](https://app.codecov.io/gh/BlueBrain/nexus-forge/pull/389/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlueBrain) | `75.65% <37.50%> (-0.05%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=BlueBrain#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

eleftherioszisis commented 7 months ago

How would it work in the case of multiple images? Just by stacking multiple lazy actions together like:

resource.image = [forge.attach_image(path, content_type, about) for path in paths]

?

MFSY commented 7 months ago

I think this capability should go to Resource or one of its descendants just like Dataset currently holds specific data models for distributions and provenance. It is okay to add more data models for specific properties we want to offer as default on Resource or on one of its descendants.

forge.attach already supports uploading a dir (you'll get a list of distributions DataDownload)

crisely09 commented 7 months ago

I think this capability should go to Resource or one of its descendants just like Dataset currently holds specific data models for distributions and provenance.

@MFSY my issue with adding it to Resource is that we would need to pass forge to it, as it is done in Dataset, and that is kind of ugly. Also because that would mean a big change in how we have been initializing Resource. Maybe passing forge only to the method add_image ?

crisely09 commented 7 months ago

How would it work in the case of multiple images? Just by stacking multiple lazy actions together like:

resource.image = [forge.attach_image(path, content_type, about) for path in paths]

?

In fact, the path can be a directory, and that would take all the files inside, and make the list automatically

MFSY commented 7 months ago

I think this capability should go to Resource or one of its descendants just like Dataset currently holds specific data models for distributions and provenance.

@MFSY my issue with adding it to Resource is that we would need to pass forge to it, as it is done in Dataset, and that is kind of ugly. Also because that would mean a big change in how we have been initializing Resource. Maybe passing forge only to the method add_image ?

I am okay with having the method accept a forge object. Now Resource can also be extended with an Entity class that will have this method which can accept forge. In that case, client will need to create an Entity from a resource before using the method.

AurelienJaquier commented 7 months ago

How would it work in the case of multiple images? Just by stacking multiple lazy actions together like:

resource.image = [forge.attach_image(path, content_type, about) for path in paths]

?

In fact, the path can be a directory, and that would take all the files inside, and make the list automatically

What if we have files from different directories? Would the piece of code provided by elefterios work?

crisely09 commented 7 months ago

How would it work in the case of multiple images? Just by stacking multiple lazy actions together like:

resource.image = [forge.attach_image(path, content_type, about) for path in paths]

?

In fact, the path can be a directory, and that would take all the files inside, and make the list automatically

What if we have files from different directories? Would the piece of code provided by elefterios work?

I think it should, yes

darshanmandge commented 5 months ago

Hi @crisely09! Can this PR be merged now?

stefanoantonel commented 4 months ago

Any update on this merge?

MFSY commented 4 months ago

Hi guys, We'll look into this this week.

crisely09 commented 4 months ago

With the current changes, to add an image, one has to start from a dataset:

dataset = Dataset.from_resource(forge, resource)
dataset.add_image(path, content_type, about)

This should work in the same way as explained before for paths and for a list of files.