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

Updating CMORization episode #313

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Updating CMORization episode #313

wants to merge 10 commits into from

Conversation

gulcing
Copy link

@gulcing gulcing commented May 2, 2024

Pull Request checklist

We appreciate your time and effort to improve the tutorial. Please keep in mind that lesson maintainers are volunteers and it may be some time before they can respond to your contribution.

Before you start

  • Read CONTRIBUTING.md.
  • Create an issue to discuss your idea. This allows your contributions to be incorporated into the tutorial.

Tasks

  • Give this pull request a descriptive title.
  • If you are contributing to existing lesson materials, please make sure the content conforms to the Lesson development section in CONTRIBUTING.md and does not contain any spelling or grammatical errors.
  • If you are making a new episode, please make sure the content conforms to the Lesson organization and Lesson formatting sections in CONTRIBUTING.md and does not contain any spelling or grammatical errors.
  • Preferably Codacy checks pass. Status can be seen below your pull request. If there is an error, click the link to find out why.
  • Preview changes on your machine before pushing them to GitHub by running make serve, alternatively make docker-serve. Please see the Previewing your changes locally section in CONTRIBUTING.md for installation instructions.
  • All code instructions have been tested.

If you need help with any of the tasks above, please do not hesitate to ask by commenting in the issue or pull request.

Closes {#270 #289 #314}

@gulcing gulcing added feature Suggest a feature enhancement New feature or request labels May 2, 2024
@gulcing gulcing requested a review from LisaBock May 2, 2024 12:02
Copy link
Contributor

@LisaBock LisaBock left a comment

Choose a reason for hiding this comment

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

Thanks @gulcing for adding these information! This seems very useful.

@LisaBock
Copy link
Contributor

@gulcing :
I rerun all code which is in this episode and it works all fine for me.
Maybe we could have a look together on your error message...

@LisaBock LisaBock requested a review from rswamina May 28, 2024 13:19
@LisaBock
Copy link
Contributor

Hi @rswamina , could you maybe have a look on this PR?

Strangely the recent updates from you in the basic episodes are shown here as well. I don't know why...

@@ -123,6 +123,12 @@ run the CMORizer scripts:
esmvaltool data format --config_file <path to config-user.yml> <dataset-name>
```

The options `--start` and `--end` can be added to command above to restrict the
formatting of raw data to a time range. They will be ignored if a specific
dataset does not support (i.e. because it is provided as a single file).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change: dataset does not support -> dataset does not support this option (i.e because all the data is provided as a single file).

... input_dir = /home/peter/data/RAWOBS
... output_dir = /home/peter/esmvaltool_output/data_formatting_20220726_140216
... input_dir = /work/bd0854/DATA/ESMValTool2/RAWOBS
... output_dir = /scratch/b/b309059/esmvaltool_output/data_formatting_20240527_132448
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not clear if b309059 is a user name. I suggest changing this to username and mentioing that this will be substituted by the individual's username (maybe at the start of the episode).

~~~
{: .error}

If you look closely at the error messages, you can see that this error concerns
the units of the coordinates. ESMValTool tries to fix them automatically,
If you look closely at the error messages, you can see that these error concern
Copy link
Contributor

Choose a reason for hiding this comment

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

suggested change: that these error concern -> the reasons for these errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

@LisaBock - I am not sure why example_output.txt was touched with this PR as it is not used in the CMORization episode. This is an older version of the file I think. Can you update this from the main repository before pushing?
I have suggseted some changes. If you are ok with them, let me know once when done and I will approve.

@LisaBock
Copy link
Contributor

@rswamina - I realized today that we were already almost done with updating the cmorization episode. So I made some changes according to your review and updated the compatibility to v2.11.0. Should be fine now. If you agree, please merge.

...
... Starting the CMORization Tool at time: 2022-07-26 14:02:16 UTC
... Writing program log files to:
/scratch/b/b309059/esmvaltool_output/data_formatting_20240527_132448/run/main_log.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

Can all references to b309059 be changed to username so it is less confusing and consistent with other episodes?

Copy link
Contributor

@rswamina rswamina left a comment

Choose a reason for hiding this comment

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

@LisaBock - Is it possible for me to review one final version of this document. Right now I see a plit screen with some old and newer changes. If you can make changes to alter the username and push teh changes, I will take another pass at reviewing.

@rswamina
Copy link
Contributor

@LisaBock and @gulcing - can you add a link to the documentation for this tutorial. Maybe this page : https://docs.esmvaltool.org/en/latest/develop/dataset.html .

@LisaBock
Copy link
Contributor

@LisaBock - Is it possible for me to review one final version of this document. Right now I see a plit screen with some old and newer changes. If you can make changes to alter the username and push teh changes, I will take another pass at reviewing.

@rswamina - The PR is ready now for a final review...

@LisaBock
Copy link
Contributor

@LisaBock and @gulcing - can you add a link to the documentation for this tutorial. Maybe this page : https://docs.esmvaltool.org/en/latest/develop/dataset.html .

@rswamina - good idea! I will do so when the updated episode is merged.

@rswamina
Copy link
Contributor

@LisaBock and @gulcing - can you add a link to the documentation for this tutorial. Maybe this page : https://docs.esmvaltool.org/en/latest/develop/dataset.html .

@rswamina - good idea! I will do so when the updated episode is merged.
@LisaBock - Are you going to add a link to the documentation page in this episode before I approve and merge or did you want to cerate a new PR for that?

@LisaBock
Copy link
Contributor

@LisaBock and @gulcing - can you add a link to the documentation for this tutorial. Maybe this page : https://docs.esmvaltool.org/en/latest/develop/dataset.html .

@rswamina - good idea! I will do so when the updated episode is merged.
@LisaBock - Are you going to add a link to the documentation page in this episode before I approve and merge or did you want to cerate a new PR for that?

Ah, sorry @rswamina , I understood it wrong. I thought you want us to add a link in the documentation...
As there are already links in this episode to the documentation, could you please point me to the place where you are missing a link?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feature Suggest a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants