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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DAS-2166: Improve HyBIG documentation and notebooks #12

Merged
merged 9 commits into from
May 15, 2024

Conversation

flamingbear
Copy link
Member

Description

Add information to the HyBIG README regarding default behavior for regridding, tiling, coloring and customizations.

Update example notebook to match current HyBIG functionality

Jira Issue ID

DAS-2166

Local Test Steps

Pull this branch, create a python environment with the packages in docs/requirements.txt
Run the notebook and see that all of the output looks correct.

Read the README for clarity and understanding.

Check Notebook for typos or bad wording.

PR Acceptance Checklist

  • Jira ticket acceptance criteria met.
  • [N/A] CHANGELOG.md updated to include high level summary of PR changes.
  • [N/A] docker/service_version.txt updated if publishing a release.
  • [N/A] Tests added/updated and passing.
  • Documentation updated (if needed).

No release needed since this is only documentation related.

Copy link
Member

@owenlittlejohns owenlittlejohns left a comment

Choose a reason for hiding this comment

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

This looks great, thanks for the update.

I think there are two things I reckon could do with a quick change:

  • Tracking through the notebook and check the file names. I'm seeing a bunch of stuff locally where this notebook is looking for something like "VCF5KYR_1991001_001_2018224205008.png" but the files that are present are more like "1991001_001_2018224205008.png".
  • Making sure the GIBS-compatible default behaviour is explained. I think the origin of this ticket came from realising what the default behaviour for scale extent wasn't really stated anywhere. I think the README is possibly only missing the scale extent information, but the notebook would probably benefit for having all those defaults explained in the markdown for the first example.

README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
|- docker
|- docs
|- harmony_browse_image_generator
|- legacy-CHANGELOG.md
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for adding these missing items. (Although, now it's making me realise we're potentially missing the description of the license file in the list below, maybe the file is self-explanatory, but might be worth mentioning it's required for NASA open-source release)

Copy link
Member Author

@flamingbear flamingbear May 15, 2024

Choose a reason for hiding this comment

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

Yes I looked at all the files and was trying to decide which ones should be described. and with license it would be.

|- license

license - license file for repository

IDK. No strong opinions, but also don't think it's necessary

Copy link
Member

Choose a reason for hiding this comment

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

I think if we were to capture anything it would be that the license is in compliance with NASA open-source software requirements. Potential text:

  • LICENSE - Required for distribution under NASA open-source approval. Details conditions for use, reproduction and distribution.

Probably not perfect text, but I think that's the gist of anything I would add.

docs/HyBIG-Example-Usage.ipynb Show resolved Hide resolved
docs/HyBIG-Example-Usage.ipynb Show resolved Hide resolved
@flamingbear
Copy link
Member Author

  • I think the origin of this ticket came from realising what the default behaviour for scale extent wasn't really stated anywhere.

No, but I said I'd put that in.

@flamingbear
Copy link
Member Author

No, but I said I'd put that in.

@owenlittlejohns Ok, I added that information to the README.md I don't think I want to copy it into the notebook. I might update the notebook to alert the reader to a README.md file

Copy link
Member

@owenlittlejohns owenlittlejohns left a comment

Choose a reason for hiding this comment

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

Nice one. Thanks for the updates.

I think the only think could be tweaked at this point is bumping the version of harmony-py listed in the requirements file.

README.md Show resolved Hide resolved
|- docker
|- docs
|- harmony_browse_image_generator
|- legacy-CHANGELOG.md
Copy link
Member

Choose a reason for hiding this comment

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

I think if we were to capture anything it would be that the license is in compliance with NASA open-source software requirements. Potential text:

  • LICENSE - Required for distribution under NASA open-source approval. Details conditions for use, reproduction and distribution.

Probably not perfect text, but I think that's the gist of anything I would add.

docs/HyBIG-Example-Usage.ipynb Show resolved Hide resolved
Also:
Adds license text to readme.
Reorders file descriptions correctly.
@flamingbear flamingbear merged commit 3fda9b6 into main May 15, 2024
4 checks passed
@flamingbear flamingbear deleted the mhs/DAS-2166/expand-documentation branch May 15, 2024 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants