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

ADD: Animated GIF PPI Blog notebook #1598

Merged
merged 12 commits into from
Jun 27, 2024

Conversation

BrandonWeart
Copy link
Contributor

@BrandonWeart BrandonWeart commented Jun 18, 2024

This is the Making animated GIFs for radar PPI plots blog. This is in relation to the Summer 2024 SULI project and it covers a convective event over Lemont, IL on June 5th, 2024

  • Closes #xxxx
  • Tests added
  • Documentation reflects changes

This is the Making animated GIFs for radar PPI plots blog. This is in relation to the Summer 2024 SULI project and it covers a convective event over Lemont, IL on June 5th, 2024
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@zssherman
Copy link
Collaborator

Reopening to rerun tests now that the CI is fix.

@zssherman zssherman closed this Jun 20, 2024
@zssherman zssherman reopened this Jun 20, 2024
@zssherman
Copy link
Collaborator

zssherman commented Jun 20, 2024

@BrandonWeart Looks like some linting issues that need some cleaning
https://github.com/ARM-DOE/pyart/actions/runs/9599719708/job/26474191833?pr=1598

@zssherman
Copy link
Collaborator

zssherman commented Jun 20, 2024

@BrandonWeart Also you need to add an author and date to the metadata of ipynb file. An example is:

 "metadata": {
  "author": "Max Grover",
  "date": "2024-03-21",
  "kernelspec": {
   "display_name": "Python 3 (ipykernel)",
   "language": "python",
   "name": "python3"
  },

Yours is:

 "metadata": {
  "kernelspec": {
   "display_name": "CROCUS",
   "language": "python",
   "name": "crocusenv"
  },

And should be changed to:

 "metadata": {
  "author": "Brandon Weart",
  "date": "your_date",
  "kernelspec": {
   "display_name": "CROCUS",
   "language": "python",
   "name": "crocusenv"
  },

@BrandonWeart
Copy link
Contributor Author

I'm not sure why these linting checks aren't passing. Looking at my recent overwrite of the original file, the changes that pre-commit wants to impose here are the same ones pre-commit made on my local machine.

@zssherman
Copy link
Collaborator

@BrandonWeart Hmm that i'm not sure, let me think on that unless @mgrover1 has some thoughts?

@BrandonWeart
Copy link
Contributor Author

Yeah its kinda strange. If you look at the most recent commit I made you can see the changes.

@zssherman zssherman requested a review from mgrover1 June 24, 2024 14:09
@zssherman
Copy link
Collaborator

@mgrover1 Worked with Brandon on getting the linter stuff fixed. Metadata is in the file as well, when you have a chance for a second review.

@BrandonWeart
Copy link
Contributor Author

Hoping it's good!

Copy link

review-notebook-app bot commented Jun 25, 2024

View / edit / reply to this conversation on ReviewNB

mgrover1 commented on 2024-06-25T15:59:31Z
----------------------------------------------------------------

Within this blog post instead of within this notebook


Copy link

review-notebook-app bot commented Jun 25, 2024

View / edit / reply to this conversation on ReviewNB

mgrover1 commented on 2024-06-25T15:59:31Z
----------------------------------------------------------------

Include blog post instead of notebook here again


Copy link
Collaborator

@mgrover1 mgrover1 left a comment

Choose a reason for hiding this comment

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

Two very small comments - otherwise, looks great!

@BrandonWeart
Copy link
Contributor Author

Hey Max I found the first instance of notebook needed to be changed to "blog post" but where is the second one?

@zssherman
Copy link
Collaborator

@BrandonWeart At the very end, I think he means changing it from example to blog post in the final summary.

@mgrover1
Copy link
Collaborator

Screenshot 2024-06-27 at 11 46 47 AM @BrandonWeart - in the summary section!

@BrandonWeart
Copy link
Contributor Author

Didnt mean to commit twice. Had an issue with my laptop for a second but it should all be done!

Copy link
Collaborator

@mgrover1 mgrover1 left a comment

Choose a reason for hiding this comment

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

Thanks for making those changes! Looks good to me!

@mgrover1 mgrover1 merged commit 60655dd into ARM-DOE:main Jun 27, 2024
18 checks passed
@BrandonWeart BrandonWeart deleted the BrandonWeart-patch-1 branch June 27, 2024 20:20
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.

3 participants