-
Notifications
You must be signed in to change notification settings - Fork 45
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
Static Documentation for v0.1 #118
Conversation
UI supports vector search & arithmetic for embeddings generated from CLAY. --------- Co-authored-by: SRM <[email protected]>
Currently fail to run. I suspect @srmsoumya made this with a different version of the ClayModule, but I can't track the differences. Error is:
|
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.
Thanks, Just some suggested changes for now. Will do a proper review after #116 is merged (hopefully today), so that the diff is smaller and easier to review (22 files right now is a lot). I see also that there is duplication of documentation between the installation/basic_usage pages and the main README.md, and we should just have those instructions in one place.
@brunosan, would you like us (DevSeed) to push any changes directly to this PR branch, or would you like to handle the documentation personally for Davos? Cc @srmsoumya, as this brings in your notebooks from the docs/model
branch.
@@ -581,19 +595,22 @@ def __init__( # noqa: PLR0913 | |||
"sar": (10, 11), | |||
"dem": (12,), | |||
}, | |||
shuffle=True, |
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.
@srmsoumya, did we discuss to make shuffle to be off by default (so that people running inference can avoid surprising results), and only turn this on during training?
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.
Leaving as is for now, but created ticket #123
Yes please make all changes you feel should be done. I can take over
later today.
|
Co-authored-by: Wei Ji <[email protected]>
Co-authored-by: Wei Ji <[email protected]>
Co-authored-by: Wei Ji <[email protected]>
Co-authored-by: Wei Ji <[email protected]>
Co-authored-by: Wei Ji <[email protected]>
Co-authored-by: Wei Ji <[email protected]>
Co-authored-by: Wei Ji <[email protected]>
Make sure that large files don't get commited to git. Xref https://github.com/pre-commit/pre-commit-hooks/tree/v4.5.0#check-added-large-files
Don't force running all notebooks on the docs build, only run those that don't have output cells already.
Hosting the PNG images directly via GitHub's UI, instead of in the git repo.
Don't build files that aren't in the Table of Contents (https://jupyterbook.org/en/stable/structure/configure.html#disable-building-files-that-arent-in-the-table-of-contents). Can then rename README to README.md without raising a warning like `WARNING: document isn't included in any toctree`.
Some minor changes to the clay-v0-interpolation.ipynb file, and remove the large sample.gif file. Also gitignoring .gif and .png files.
Will be handled in #122
Should be 10 bands of Sentinel-2, not 13.
docs/_toc.yml
Outdated
- title: Generative AI for pixel reconstruction | ||
file: 240105-clay-v0-reconstruction | ||
- title: Create location embeddings | ||
file: 240105-location-embeddings | ||
- title: Interpolating images in embedding space | ||
file: 240105-clay-v0-interpolation |
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.
Probably should change these filenames, otherwise the URL comes up as https://clay-foundation.github.io/model/240105-clay-v0-reconstruction.html
which doesn't look nice.
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.
Done in 9926919
Most major issues with documentation quality have been resolved
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.
Aight, lets do this!
This PR expands the current documentation based on #64 and tries to clarify structure.
It does not include executable documentation (
Experiments
, renamedtutorials
) since I don't know yet how we can show the outputs of these without adding execution outputs to the repo history, nor having the CI download and execute the code. I'll PR a solution forking from here.It also bring over the notebook from branch
clay-over-ai
.We should have this merged before Wednesday presentation in Davos.