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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sub-folders containing model-specific information #4

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

Jordan-Pierce
Copy link

Per the discussion, this PR contains the following:

Updated Main readme

  • Added the link to the publication,
  • Added two additional rows for projects that use FathomNet data

Sub-folders for each model

  • Each model has it's own sub-folder in the root of the repo with representative name
  • Sub-folders are relatively consistent in structure
  • Sub-folders contains folders for scripts, notebooks, resources, figures, and a folder containing the original repo project to easily refer back to
  • Sub-folder's readme(s) contain basic information, links to project / publication, and status of that model

Links to HuggingSpace demos are currently public, happy to transfer ownership and make a PR within the Fathomnet HuggingFace organization.

Take what you want, gut what you don't 馃槑

@eor314 eor314 self-assigned this May 31, 2023
@eor314 eor314 added the enhancement New feature or request label May 31, 2023
@eor314 eor314 linked an issue May 31, 2023 that may be closed by this pull request
@eor314
Copy link
Contributor

eor314 commented Jun 1, 2023

I think we should merge most of this to a new branch until after we sort out our future model zoo development strategy. We should definitely merge the new model into the table in the README of main.

@eor314
Copy link
Contributor

eor314 commented Jun 2, 2023

@Jordan-Pierce I added you as a contributor to our HuggingFace community. Do you want to try to PR the Spaces you set up into the FathomNet community there? It'd be cool to get a sense of what the process looks like for potential future model sharing in the HF. Thanks!

@Jordan-Pierce
Copy link
Author

Hey @eor314, I just transferred the spaces from my account to the FathomNet account. However, I just re-read your last comment about the PR specifically. How do you feel about me creating a Squid detector space under the FathomNet account, and while it's empty, I'll make a fork, add to the space, and then submit a PR?

Copy link
Member

@kevinsbarnard kevinsbarnard left a comment

Choose a reason for hiding this comment

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

Thanks so much for the PR Jordan! I left some minor comments, but overall I like the structure you've created.

A couple overarching thoughts:

  • Including assets (images, videos) in the repo adds quite a bit of bloat, especially when they're duplicated across the different model subdirectories.
  • Each model directory lends itself to being its own self-contained repo. I'd suggest breaking each model into a separate repo, then adding them to this one as submodules. That way, we can keep the organization of the models separate from the model assets/scripts themselves. As this repo grows, it may become unwieldy to manage all of them under one roof.

Copy link
Member

Choose a reason for hiding this comment

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

We should exclude IDE configuration from being checked in to the repo -- I recommend removing the .idea directory and its contents

Copy link
Member

Choose a reason for hiding this comment

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

Duplicated assets here.

Copy link
Member

Choose a reason for hiding this comment

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

Ditto

Copy link
Member

Choose a reason for hiding this comment

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

Ditto

@Jordan-Pierce
Copy link
Author

Hey @eor314 @kevinsbarnard moving the conversation here; given the transition to HF do you just want to squash this PR? Or how would you like to proceed?

I think the only addition I have to the readme is the inclusion of the UWROV in the table, which is also apart of the FathomNet HF, might be worth adding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pre-PR Discussion
3 participants