eurec4a / how_to_eurec4a

Code examples to get you started with EUREC⁴A data.
https://howto.eurec4a.eu
MIT License
6 stars 20 forks source link

Added specMACS cloud geometry markdown example and pictures #99

Closed lvol08 closed 1 year ago

d70-t commented 1 year ago

Hey @lvol08, thanks for this valuable contribution πŸŽ‰

I noticed, that your markdown doesn't seem to be a MySt notebook. Furthermore, it contains code output and pre-generated plots. For the HowTo Eurec4a, we want to run all the code described by the book using CI pipelines, in order to verify that the code actually does what it should do. Here's a bit of a description, how the book is meant to work. If you follow that description, you'll see if the article appears correctly on the generated page.

Could you converte the notebook to MySt format and remove the generated plots and output from the repository? They'll get regenerated if the code works out.

Further, references should go into references.bib and the article would have to be included somewhere in _toc.yml to show up in the final page.

lvol08 commented 1 year ago

Hey @d70-t , thanks for the comments and sorry for not noticing in advance.

I changed the notebook to MySt format and added the references to the references.bib and included the notebook in the _toc.yml file. However, I am not sure if the notebook actually works with the given requirements for the python environment but that's unfortunately the case for all notebooks in the catalogue that use matplotlib for me. I can install matplotlib again in the environment but shouldn't it work just with installing the requirements?

d70-t commented 1 year ago

Hey @lvol08, thanks for the quick update. As far as I can see, the notebook builds fine with the given requirements (we would update them if not, but it doesn't seem to be necessary). The build is currently failing for other notebooks. And actually your notebook executes rather quickly, which is extra nice! Probably you'd have to reinstall matplotlib on your side to make it work there as well.

I'll come back soon with some more specific comments on the content, but it's already in a quite good shape πŸŽ‰ !

d70-t commented 1 year ago

One more minor thing: Before merging, we should probably do a rebase, removing the previously committed figures, to keep the repo small.

lvol08 commented 1 year ago

Hi @d70-t , thanks a lot for all the suggestions to make the code more simple and better readable! :) I think I included all the suggestions in the commits. About the rebase: Do I have to do it in my local fork, so something like "git rebase master" and place a new merge request afterwards? Thanks again!

d70-t commented 1 year ago

About the rebase: Do I have to do it in my local fork, so something like "git rebase master" and place a new merge request afterwards?

You can do it in your local fork, or I can do it for you. In any case, the result would be "force pushed" to the current branch (lvol08:specmacs_cloud_geometry) and thus the current pull request will get updated. In particular, we wouldn't want to have a new pull request, because that way we'd loose all the discussion up to now.

It would probably be an interactive rebase, e.g.:

git rebase -i master

(or origin/master or upstream/master or whatever the way you'd locally reference the current master of the main online repository)

In this interactive rebase you could squash or fixup all the commits into a single one. Afterwards, the branch would be "diverged" from the copy which is currently referenced in the pull request (because it doesn't contain the individual commits anymore) and thus to upload (and thereby replace the current commits), you'd have to use git push -f (aka force).

d70-t commented 1 year ago

The updates are working fine πŸŽ‰. The build is still failing due to #102, but that's fine for this pull request. After squashing the commits (i.e. using a rebase), it's good to go πŸ˜„. @lvol08 you decide who'll do the squash.

lvol08 commented 1 year ago

Thanks @d70-t ! I fixed one more typo, did the rebase and the force push, hope that it works all out :)

d70-t commented 1 year ago

I just did another rebase on the (again) updated master, thus the ongoing fixes from the other issues are now incorporated and the pipeline is finally green :-)

d70-t commented 1 year ago

Haha, I screwed it up... Will fix it it a moment 🀦