open_firev2

[MateoLostanlen]

Open_fire v1 is dying due to too many dead links as discuss in issue #118. I want to propose here a Open_fire V2. I have a dataset of 9000 images from which a good part comes from openfire. I propose that this dataset becomes the new Open_fire. It is downloadable from my drive so no problem with dead link here.

I guess this PR should close #9 too.

PS: For testing purposes I have created a sample version that can be downloaded quickly.
PPS: The real version of the datatset is not yet on my drive I have to put it in the next coming days but I have wifi problems

After this PR I will use the sample template for Wilfire dataset

[codecov]

Codecov Report

Merging #121 (2a71cc6) into master (40ddf2f) will decrease coverage by 1.12%.
The diff coverage is 85.00%.

❗ Current head 2a71cc6 differs from pull request most recent head eddfbeb. Consider uploading reports for the commit eddfbeb to get more accurate results

@@            Coverage Diff             @@
##           master     #121      +/-   ##
==========================================
- Coverage   92.33%   91.20%   -1.13%     
==========================================
  Files          18       18              
  Lines         652      637      -15     
==========================================
- Hits          602      581      -21     
- Misses         50       56       +6     

Flag	Coverage Δ
unittests	91.20% <85.00%> (-1.13%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pyrovision/datasets/utils.py	84.61% <ø> (-6.16%)	⬇️
pyrovision/datasets/openfire.py	85.71% <85.00%> (-5.32%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ce4db67...eddfbeb. Read the comment docs.

[frgfm]

Thanks for the PR and the dataset!
Though, GDrive is not exactly the best solution for dataset storage. How big is it?
I would rather consider putting it in a release attachment and add integrity check 👌

[MateoLostanlen]

@frgfm 500Mb, it seems too big : https://docs.github.com/en/github/managing-your-work-on-github/file-attachments-on-issues-and-pull-requests

[frgfm]

@MateoLostanlen 500Mb is quite alright :)

• first, the limitation you are mentioning is about attachment to a comment/PR/issues. I'm positive the limitation for releases are higher since I have model checkpoints of my own bigger than 150Mb
• worst case scenario if it's a bit too big, we can easily split it in 2/3 parts and put each part in the release attachments !

[MateoLostanlen]

Hi @frgfm, what do you thinks of these last changes ?

[frgfm]

Thanks for the PR! I have many questions as I'm not familiar with some classes that you used in here 😅

[frgfm]

@MateoLostanlen sorry I thought that you saw, but I didn't review because there is still a failing test which is related to this PR. Would you mind fixing it before moving on to the review? 🙏

[MateoLostanlen]

@frgfm oh sorry this pr is open for too long I had forgotten :) It's ok the error is fixed. There is still a problem but it has nothing to do with pyrovision as explained here

[frgfm]

Thanks! My bad, I had to jump back into this

[frgfm]

The unittests looks good thanks!

For now, about the legal part of sharing the URL of the dataset, I can suggest a viable option : we don't share the URL, we only specify about the folder should be structured. People make their own dataset and they'll have to pass the path to the extracted archive.

(while we figure out how/if we can share this)

[MateoLostanlen]

I thinks it will be complicated in a legal point of view . I think our current version it's not legal either. So let's delete it from the repo. However if you do not download any images, this dataset became exactly the ImageFolder dataset from torchvision. It is still usefull ?

[frgfm]

I thinks it will be complicated in a legal point of view . I think our current version it's not legal either. So let's delete it from the repo. However if you do not download any images, this dataset became exactly the ImageFolder dataset from torchvision. It is still usefull ?

Fair point

For now, perhaps we only use ImageFolder for training and it will simplify things until we can share a public dataset

[frgfm]

In this case, there are a few things missing:

• removing openfire from datasets/init.py
• updating the reference script to use ImageFolder

[MateoLostanlen]

Done :)

[frgfm]

Thanks a lot 🙂
Perhaps we should document the structure of the folder but it's nitpicking, no problem for me to move forward with this 👍

[frgfm]

Perhaps we should specify the folder expected structure / hierarchy?

[frgfm]

Overruled by #153 :)
OpenFire is back 👍