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 P3 examples #52

Merged
merged 30 commits into from
May 11, 2021
Merged

Add P3 examples #52

merged 30 commits into from
May 11, 2021

Conversation

RobertPincus
Copy link
Contributor

Adds a few hopefully interesting examples focusing on measurements from the P-3. Uses the colorcet, seaborn and especially cartopy Python modules. Cartopy has many dependencies so modules are now installed via conda for the Continuous Integration. Adding cartopy closes #50 and may be relevant to #11.

Probably best to squash-and-merge as no one needs to see my stumbling around.

@RobertPincus RobertPincus requested a review from d70-t April 27, 2021 17:11
@RobertPincus
Copy link
Contributor Author

RobertPincus commented Apr 27, 2021

To install Python dependencies with conda: conda env create -f environment.yml will create an virtual environment how_to_eurec4a. Using source activate how_to_eurec4a or conda activate how_to_eurec4a will make the installed modules available.

@RobertPincus
Copy link
Contributor Author

... and probably we need to update gitlab-ci.yml to install dependencies and invoke the virtual environment with conda rather than `pip.

@RobertPincus
Copy link
Contributor Author

@tmieslinger In case you have the bandwidth to look over these changes (@d70-t being quite busy it seems), I would prefer not to merge them until someone else had a look at them.

Note that the cell timeout is currently set to 150 seconds; this sometimes is too short for the P-3 flight track cells.

Also - we're now using conda to manage Python dependencies in the Github CI but I don't know how to make this change on the Gitlab which builds the book for distribution.

@d70-t
Copy link
Collaborator

d70-t commented Apr 30, 2021

Sorry for not responding yet. I generally like the changes a lot, but had to do something else the last two days. I've got a few comments and will likely post a partial review soon.

@RobertPincus
Copy link
Contributor Author

Of course, @d70-t, and no worries. Not everything should fall on your shoulders.

I quite liked your cleanup ideas, I have to say (#47)

Copy link
Collaborator

@d70-t d70-t left a comment

Choose a reason for hiding this comment

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

Dear @RobertPincus,

thank you very much for all these new book chapters. I think they are really valuable and nicely made! 👍 In particular I like how you describe all the things, that is exactly what we need.

I've quite a few technical comments to the code. I've split those up into individual suggestions and comments such that we could discuss about them. I hope they are consistent, but probably I've missed something.
I only had a deeper look at the AXBTs for now, but from quickly looking over the others, I think that some of those suggestions will apply to others as well.

I'll leave this review as is for now and have a look at some other open things for a moment.

Apart from those, we'll need:

  • an update in the "running locally" chapter which describes how to set up the conda based environment. As I also don't want to force others to install conda only to build the book, there should still be an option to install via pip. I think it is ok to state that if someone wants to use pip, one needs to take care of the cartopy installation requirements beforehand and put a link to their documentation.
  • we should have another CI job which ensures that installing via pip still works.

environment.yml Outdated Show resolved Hide resolved
requirements.txt Outdated Show resolved Hide resolved
how_to_eurec4a/p3_AXBTs.md Outdated Show resolved Hide resolved
how_to_eurec4a/p3_AXBTs.md Outdated Show resolved Hide resolved
how_to_eurec4a/p3_AXBTs.md Outdated Show resolved Hide resolved
how_to_eurec4a/pthree.md Outdated Show resolved Hide resolved
how_to_eurec4a/p3_AXBTs.md Outdated Show resolved Hide resolved
how_to_eurec4a/p3_AXBTs.md Outdated Show resolved Hide resolved
import eurec4a
cat = eurec4a.get_intake_catalog()
```
Mapping takes quite some setup. Maybe this should become part of the `eurec4a` Python module.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably. However I am yet undecided if the eurec4a module should grow larger than accessing EUREC4A data and metadata. I think functions like those should either be in a generic campaign data analysis package or we should make chapters of the book importable (#28).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure the best way forward. All the mapping stuff in my contributions comes from Bjorn in any case. It does seem strange to have to repeat code in every document independently.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I tend to go towards importable book chapters, but I am afraid that I won't be able to do that right now... Thus for now, we probably need to stick a little with repeated code.

requirements.txt Outdated Show resolved Hide resolved
@RobertPincus
Copy link
Contributor Author

@d70-t Cool, thanks, I'll take a look. The book chapters are just excerpts from the notebook that generates figures for the paper, which is why there's overlap...

@RobertPincus
Copy link
Contributor Author

@d70-t How should we proceed with cartopy and pip? I understand the issue to be that pip doesn't install all the needed dependencies, and that we expect people to have done this on their own.

@d70-t
Copy link
Collaborator

d70-t commented May 1, 2021

Initially I thought the best solution might be to include cartopy in requirements.txt as this would require one step less to install manually (users would only have to install cartopys dependencies, not cartopy itself manually).

But now I am thinking, maybe it is actually better not to specify it in requirements.txt, because that way, the installation won't fail if the dependencies of cartopy are not there. In stead only those notebooks which actually use cartopy will fail. If someone wants to run / edit the book without having cartopy installed, this would then still be possible (one would have to ignore some build errors though, but that's probably ok?).

If a user who doesn't use conda is more in the cartopy-business, I think it is not significantly more complex to install cartopy's dependencies and cartopy itself.

I'd suggest to update the running locally document (which should soon turn into a few more chapters describing how to contribute, prepare datasets etc...) such that it contains at least a description on how to set the book up using conda. I'd also keep the description on pip and add a short note that one should try to get cartopy running as well. (This might be as simple as apt install python3-cartopy or something similar using brew, but I'd not go into further details. For the two alternatives, tabbed blocks or panels might be handy. I'd be happy if you could write something about conda, because I really don't know that thing. If you don't want to mess too much on pip here, I'd suggest to make this PR such that it runs smoothly on conda and is properly documented. I'll then create a new PR on top of this which tries to get a new pip based thing running again as well.

@RobertPincus
Copy link
Contributor Author

@d70-t I added material to the chapter on running locally to describe the use of conda. You can let me know if it's not clear. I don't mention, but maybe I should, that conda is much slower than pip, partly but not exclusively because it's managing cartopy and its dependencies.

@d70-t
Copy link
Collaborator

d70-t commented May 3, 2021

As @tmieslinger noted, it seems like I've forgotten to include the color_of_day function in the review:

def color_of_day(day):
    return plt.cm.viridis(norm(day), alpha=0.9)

Copy link
Collaborator

@tmieslinger tmieslinger left a comment

Choose a reason for hiding this comment

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

@RobertPincus thank you so much for this nice contribution to the book! I had great fun reading the Ms Piggy chapters :-) they are easy to follow and are a nice complement to the P3 data paper.

I have some really minor comments on top of @d70-t's. Currently, I favor the idea of having a separate domain name for the book and linking that to GitHub pages as discussed in the issue #53 . As far as I understand we could then get rid of the GitLab integration, which means that we don't have to adjust the GitLab workflow to the new conda setup. In the GitHub workflow we should ideally include another job for building the book with pip only. And finally, we could merge this PR while being on the save side that the book compiles and people understand the conda/pip difference? Did I understand that correctly?

Seems like a long detour that would however really improve the book :)

how_to_eurec4a/p3_AXBTs.md Outdated Show resolved Hide resolved
how_to_eurec4a/p3_AXBTs.md Outdated Show resolved Hide resolved
how_to_eurec4a/p3_wband_radar.md Outdated Show resolved Hide resolved
@RobertPincus
Copy link
Contributor Author

@d70-t @tmieslinger Thanks for your very helpful comments. I believe they are all incorporated including a little bit of extra explanation about how to run locally. I've explained that users need to manage the installation of cartopy themselves.

I have not updated the gitlab-ci to work with conda so the book won't publish automatically until/unless you adopt the Github publishing @d70-t is working on with #53 .

As of now I won't plan to make further changes until the publishing issue is resolved, but it would be nice to have this folded in sooner than later.

@RobertPincus
Copy link
Contributor Author

I would suggest that you add yourselves as co-authors to the commit.

Copy link
Collaborator

@d70-t d70-t left a comment

Choose a reason for hiding this comment

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

Thanks @RobertPincus for all those updates. I've now also reviewed the other notebooks as well. I've got one major speedup and a few more nitpicks for you. Otherwise, I think we only have to wait for the DNS entry for howto.eurec4a.eu to become live (I already asked for it, but am still waiting for an answer) and then we are all set for a merge. 🎉

import eurec4a
cat = eurec4a.get_intake_catalog()
```
Mapping takes quite some setup. Maybe this should become part of the `eurec4a` Python module.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I tend to go towards importable book chapters, but I am afraid that I won't be able to do that right now... Thus for now, we probably need to stick a little with repeated code.

how_to_eurec4a/p3_AXBTs.md Outdated Show resolved Hide resolved
how_to_eurec4a/p3_flight_tracks.md Outdated Show resolved Hide resolved
how_to_eurec4a/p3_humidity_comparison.md Outdated Show resolved Hide resolved
how_to_eurec4a/p3_wsra.md Outdated Show resolved Hide resolved
how_to_eurec4a/p3_wsra.md Outdated Show resolved Hide resolved
how_to_eurec4a/_config.yml Outdated Show resolved Hide resolved
@RobertPincus
Copy link
Contributor Author

@d70-t The speedup was an excellent suggestion, thanks a lot, and I think all the changes are in place. The CI is failing in the HAMP chapter for reasons I don't understand, but we can re-run the CI before merging once the domain is in place.

@d70-t
Copy link
Collaborator

d70-t commented May 5, 2021

The HAMP files have been moved. That should be fixed in eurec4a/eurec4a-intake#67, but maybe I'll have to add another quick fix to the book as the version update could lead to a minor issue in some of the chapters... I'll sort it out.

@RobertPincus
Copy link
Contributor Author

@d70-t @tmieslinger I brought the P-3 pull request up to date with the excellent changes on master. I hesitate to squash-and-merge because I don't understand what will happen to the Gitlab CI, which uses pip without cartopy, while the Github CI and publication uses conda

@d70-t d70-t merged commit 59cf457 into eurec4a:master May 11, 2021
@RobertPincus RobertPincus deleted the add-p3-examples branch May 14, 2021 18:14
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.

How to add cartopy?
3 participants