Skip to content

Initial Cartopy work #24

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

Merged
merged 15 commits into from
Apr 22, 2021
Merged

Initial Cartopy work #24

merged 15 commits into from
Apr 22, 2021

Conversation

ktyle
Copy link
Contributor

@ktyle ktyle commented Mar 29, 2021

Created first Cartopy notebook and revised _config.yml and _toc.yml to reflect its addition.

@andersy005
Copy link
Member

@ktyle, to fix the CI failure, add the following to the environment.yml file:

- sqlalchemy<1.4

@ktyle
Copy link
Contributor Author

ktyle commented Mar 29, 2021

@andersy005 Thanks ... yes, I ran into this problem when setting notebook execution to "cache" in my local environment, as mine is now using sqlalchemy 1.4.2. In its setup.py, jupyter-cache specifies sqlalchemy~=1.3.12. Based on my reading of version specifications, specifically compatible-release, I thought that the only problem would be if the major version (1) changed, but that seems to not be the case.

@github-actions
Copy link

🚀 📚 Preview for git commit SHA: f049a02 at: https://606298a027be9f46aaa5850b--pythia-foundations.netlify.app

@github-actions
Copy link

🚀 📚 Preview for git commit SHA: 1286c77 at: https://606325605020ac121a2f5779--pythia-foundations.netlify.app

@brian-rose
Copy link
Member

Awesome, it's up now and working! It will be great to discuss this content with the team at the EWG meeting this week.

@github-actions
Copy link

🚀 📚 Preview for git commit SHA: d1de2e9 at: https://60633ccf8170e500baebf3cc--pythia-foundations.netlify.app

@ktyle
Copy link
Contributor Author

ktyle commented Mar 30, 2021

@dopplershift @andersy005 any tips on how to deal with the linting failures?

@brian-rose
Copy link
Member

@dopplershift @andersy005 any tips on how to deal with the linting failures?

Are you using the pre-commit hooks, as outlined on the README page? I'm not sure if these work on notebooks files.

@andersy005
Copy link
Member

I'm not sure if these work on notebooks files.

They do

- repo: https://github.com/nbQA-dev/nbQA
rev: 0.5.7
hooks:
- id: nbqa-black
additional_dependencies: [black==20.8b1]
- id: nbqa-pyupgrade
additional_dependencies: [pyupgrade==2.7.3]
- id: nbqa-isort
additional_dependencies: [isort==5.6.4]

@ktyle, as @brian-rose pointed out, try following the steps in the README. If the pre-commit hooks don't pass locally, let me know.

@andersy005
Copy link
Member

@andersy005 Thanks ... yes, I ran into this problem when setting notebook execution to "cache" in my local environment, as mine is now using sqlalchemy 1.4.2. In its setup.py, jupyter-cache specifies sqlalchemy~=1.3.12. Based on my reading of version specifications, specifically compatible-release, I thought that the only problem would be if the major version (1) changed, but that seems to not be the case.

@ktyle, you are right. The failure is due to the conda package which doesn't enforce this condition but uses sqlalchemy >=1.3.12: https://github.com/conda-forge/jupyter-cache-feedstock/blob/9d781ecd8f2a469e7315053c6661712793d87ee3/recipe/meta.yaml#L30

@andersy005 andersy005 added the content Content related issue label Mar 30, 2021
@andersy005 andersy005 requested a review from a team March 30, 2021 16:27
Copy link
Contributor

@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.

Looks great! Thanks @ktyle for putting this together.

@ktyle
Copy link
Contributor Author

ktyle commented Mar 30, 2021

I'm not sure if these work on notebooks files.

They do

- repo: https://github.com/nbQA-dev/nbQA
rev: 0.5.7
hooks:
- id: nbqa-black
additional_dependencies: [black==20.8b1]
- id: nbqa-pyupgrade
additional_dependencies: [pyupgrade==2.7.3]
- id: nbqa-isort
additional_dependencies: [isort==5.6.4]

@ktyle, as @brian-rose pointed out, try following the steps in the README. If the pre-commit hooks don't pass locally, let me know.

Yeah, I should have read the README, who knew!! :) I have now built a local env using the prescribed pythia-book-dev environment. My CentOS 7 system has versions of git and apparently nodeJS that are incompatible with the pre-commit hooks; updating git has resolved most of them but it looks like I need a newer nodeJS as well ... hopefully I'll have this all working locally shortly!

@github-actions
Copy link

🚀 📚 Preview for git commit SHA: e85fff0 at: https://6063d557c4193f38a93c9064--pythia-foundations.netlify.app

@ktyle
Copy link
Contributor Author

ktyle commented Mar 31, 2021

So the pre_commit linting step is failing, with a not-so-pretty error in the "prettier" pre_commit stage. I suspect it's the same error that occurred on my local development machine:

`/nfs/kt11/ktyle/.cache/pre-commit/repoxzxoqnu3/node_env-system/lib/node_modules/prettier/bin-prettier.js:783
return (x[0] - y[0]) ** 2 + (x[1] - y[1]) ** 2 + (x[2] - y[2]) ** 2;
^

SyntaxError: Unexpected token *
at createScript (vm.js:56:10)
at Object.runInThisContext (vm.js:97:10)
at Module._compile (module.js:549:28)
at Object.Module._extensions..js (module.js:586:10)
at Module.load (module.js:494:32)
at tryModuleLoad (module.js:453:12)
at Function.Module._load (module.js:445:3)
at Module.runMain (module.js:611:10)
at run (bootstrap_node.js:394:7)
at startup (bootstrap_node.js:160:9)`

but upgrading NodeJS to 14.16.0 seemed to resolve it. What version of NodeJS is Github using? Or is there a way we can change the behavior of pre_commit so as to control what "prettier" does (looks like there is an .eslintignore file that one can specify files to ignore, a la .gitignore)?

@andersy005
Copy link
Member

but upgrading NodeJS to 14.16.0 seemed to resolve it. What version of NodeJS is Github using?

I think the default nodeJS version in GitHub action is v12.

Or is there a way we can change the behavior of pre_commit so as to control what "prettier" does (looks like there is an .eslintignore file that one can specify files to ignore, a la .gitignore)?

You can add a .prettierignore file in the root directory, and then you can add file patterns or file paths you want prettier to ignore.

@ktyle
Copy link
Contributor Author

ktyle commented Mar 31, 2021

@andersy005 I installed a version 12 NodeJS on my local system but did not get the "prettier" error (my previous local failure was using a version 6 NodeJS).

I'm unclear exactly what prettier's role is here ... I am assuming it is markdown related. I suppose a workaround to get the linting task to pass on Github would be to add an appropriate directory to .prettierignore (which I assume would be where my 01_Cartopy notebook is), but that would also seem to defeat the purpose of using prettier.

@brian-rose
Copy link
Member

brian-rose commented Apr 1, 2021

Trying something out here. When I tried to commit locally, the pre-commit hooks were complaining like this:

nbqa-isort...............................................................Failed
- hook id: nbqa-isort
- exit code: 1

Mutation detected, will not reformat!

After some digging, I added the line

isort = 1

to the [tool.nbqa.mutate] section of pyproject.toml. As I understand it, this gives the nbqa tool the ability to modify the notebook when it encounters issues flagged by isort (which looks at your Python import statements).

Once this was in place, trying to commit resulted in changes to the import statements at the top of the notebook. With these changes staged, the commit then went smoothly on my local machine.

@brian-rose
Copy link
Member

... but that does not seem to have made the GitHub listing problems go away.

@github-actions
Copy link

github-actions bot commented Apr 1, 2021

🚀 📚 Preview for git commit SHA: effd5b2 at: https://606629871d2c5145a55bc610--pythia-foundations.netlify.app

@github-actions
Copy link

github-actions bot commented Apr 1, 2021

🚀 📚 Preview for git commit SHA: 8ac420b at: https://60662ddaf42fc34f12439333--pythia-foundations.netlify.app

@brian-rose
Copy link
Member

Wow, was prettier actually choking on the commented out lines in _config.yml?

@github-actions
Copy link

github-actions bot commented Apr 2, 2021

🚀 📚 Preview for git commit SHA: 9d79a81 at: https://60671b68c54c8daa4fbd17b1--pythia-foundations.netlify.app

@ktyle
Copy link
Contributor Author

ktyle commented Apr 2, 2021

@brian-rose Looks that way! It wanted the # lines lined up to whatever stanza they were used in.

Now I think I finally have the hang of all the workflow steps involved in pre-commit.

@brian-rose
Copy link
Member

For everyone's reference, I left an alternate version of this Cartopy content over in #26. Same content, different formatting in the source notebook. This might help folks see better how JupyterBook will render notebook content.

@clyne
Copy link
Contributor

clyne commented Apr 6, 2021

This is awesome @ktyle . Nicely done! A few comments. Some of these are specific to this document, but others are more general and we might want to discuss as a group (or get more input here).

  1. Should Matplotlib be listed as a prerequisite? On a broader note, maybe all of our internal content should have a section on suggested prerequisites.
  2. Another reference that I would suggest for the References section is NCAR's GeoCAT example plotting gallery , particularly the section on Map Projections.
  3. Should all of our book "chapters" contain a link to the conda environment instructions needed to ensure that readers can run the examples in their own environment?
  4. Our getting started draft documentation Spinup #20 has made a point of saying that you can use Python from a terminal, from a jupyter notebook, or an IDE. The examples in this chapter won't generate plots if run from a terminal because of the differences in how terminal and notebook interpreters behave. Perhaps we need to warn the reader of that? If not here, maybe in the Matplotlib primer.
  5. It would be nice of the comments (and code) in the cells were short enough so that horizontal scroll bars weren't necessarily. Maybe it is just be, but I find these pretty annoying.
  6. In section 3 it says "this time we'll define an object to store our projection definition". Could you elaborate on why someone might want to "store our definition"?
  7. I think it is a little unclear on when you explicitly need to call 'fig' and when you do not. Could you clarify?
  8. In cell 10 you are defining cLat and cLon, but you never use them. Should these be removed?

@github-actions
Copy link

🚀 📚 Preview for git commit SHA: 1ebb709 at: https://607dcc63a324060092e8d998--pythia-foundations.netlify.app

@github-actions
Copy link

🚀 📚 Preview for git commit SHA: f6dab75 at: https://607e0eb353378900956c37c0--pythia-foundations.netlify.app

@ktyle
Copy link
Contributor Author

ktyle commented Apr 20, 2021

@clyne I think I've addressed most of your comments above (not 3 or 4 yet, but I think these can be addressed once we have some Matplotlib content) with my latest push.

@brian-rose
Copy link
Member

I think I fixed a merge conflict on the environment.yaml file. Hopefully I didn't break anything.

@github-actions
Copy link

🚀 📚 Preview for git commit SHA: 6109559 at: https://6080db01c20081195d395ca7--pythia-foundations.netlify.app

@brian-rose
Copy link
Member

Per our discussion at last week's EWG, we should merge this now unless there's other feedback.

I think it would be great to merge just for stylistic comparison against all the new Unidata content discussed in #42

@ktyle
Copy link
Contributor Author

ktyle commented Apr 22, 2021

Thanks @brian-rose , yes, I think we should merge now!

@brian-rose
Copy link
Member

Ok, merging! Thanks @ktyle

@brian-rose brian-rose merged commit d326702 into ProjectPythia:main Apr 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content Content related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants