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

Better documentation for the templates #232

Closed
RondeauG opened this issue Aug 3, 2023 · 8 comments · Fixed by #235
Closed

Better documentation for the templates #232

RondeauG opened this issue Aug 3, 2023 · 8 comments · Fixed by #235
Labels
documentation Improvements or additions to documentation

Comments

@RondeauG
Copy link
Collaborator

RondeauG commented Aug 3, 2023

Generic Issue

  • xscen version: 0.6.15

Description

I've been helping @vindelico get accustomed to xscen in the last few weeks, and he had a few suggestions for our templates (especially the first 1, which is the most complete one).

  1. Be as exhaustive as possible.
    In some places in the config, we only write the arguments that are being overwritten. The reason why some arguments are missing wasn't clear. Something like this would be preferable:
  climatological_mean: # automatically passed to the function
    window: 30
    min_periods:  # Default value, so you would not need to include it in a real configuration file.
    interval: 10
    periods: [['1951', '2100']]
    to_level: climatology
  1. Better explain why certain functions (i.e. compute_deltas) are under specific categories (i.e aggregate)
# Functions under this section are associated to xscen/aggregate.py
aggregate:
  dask:
    n_workers: 4
    threads_per_worker: 4
    memory_limit: "6GB"
  ...
  1. Better order/indicate which function gets sent automatically to xscen through parse_config and which doesn't.
aggregate:
  # These entries do not correspond to an `xscen` function, but if you plan to have multiple variations of them
  # (i.e. to fine-tune dask workers depending on the task to perform), then categorizing them under `aggregate` 
  # makes it easier to locate them within the CONFIG object when you want to perform tasks related to the
  # `aggregate` module, such as when computing `climatological_mean`
  dask:
    n_workers: 4
    threads_per_worker: 4
    memory_limit: "6GB"
  input:
    clim:
      processing_level: indicators
    delta:
      processing_level: climatology
  save:
    mode: 'o'

  # These entries exactly match `xscen` functions located under the `aggregate` module, so the parameters 
  # specified below will automatically override default values. 
  # See https://xscen.readthedocs.io/en/latest/notebooks/6_config.html for more details on how it works.
  climatological_mean: 
    window: 30
    interval: 10
    periods: [['1951', '2100']]
    to_level: climatology
  compute_deltas: 
    kind: "+"
    reference_horizon: "1991-2020"
    to_level: 'delta'

@RondeauG RondeauG added the documentation Improvements or additions to documentation label Aug 3, 2023
@juliettelavoie
Copy link
Contributor

I can help with this.

@juliettelavoie
Copy link
Contributor

juliettelavoie commented Aug 4, 2023

  1. I am not convinced this is a good idea. It would make the config wayyy longuer and harder to read. The default being used when not passing an argument is not a weird behavior.
  2. All the functions that were already marked by "# automatically passed to the function". I am adding some text at the beginning to clarify the difference btw automatically passed and manually.

@vindelico
Copy link
Collaborator

vindelico commented Aug 7, 2023

  • I am not convinced this is a good idea. It would make the config wayyy longuer and harder to read.

Why not use this file for exhaustive instructions?

I started out with this config to use xscen and you have to work with it a lot to get the workflow to run.
So I went ahead and inserted what I think would be helpful for newcomers. I would also suggest (as commented) to put similar exhaustive comments in the first task of the workflow.

The default being used when not passing an argument is not a weird behavior.

I agree. However, it would be useful to showcase where this 'invisible' action is taking place in the workflow/config.

  • All the functions that were already marked by "# automatically passed to the function". I am adding some text at the beginning to clarify the difference btw automatically passed and manually.

Nice, I edited that a bit, too.

@juliettelavoie
Copy link
Contributor

I'm probably too close to it and assumed people will know the options after running through the notebooks, but you are probably right. I can add commented defaults in the config for people who start directly with the template.

@vindelico
Copy link
Collaborator

vindelico commented Aug 7, 2023

I'm probably too close to it and assumed people will know the options after running through the notebooks, but you are probably right. I can add commented defaults in the config for people who start directly with the template.

I think @RondeauG mentioned that the docs / notebooks point to the templates somewhere already. I would really see the template(s) as another angle for learning and a good place to add details that the notebooks don't cover.
I notice that I was hitch-hiking on your PR #233 for unrelated modifications.
We should have a PR dedicated to bonification of the template(s).

@juliettelavoie
Copy link
Contributor

Interesting idea. I really saw the templates as going after the notebooks.
(Though,I just noticed that "workflow templates" is before "Good to know" and any notebooks on the RtD.)
We should discuss this further at the meeting: what is the recommanded entry point ?

One thing I will say is that it much easier to showcase different arguments in the notebooks.
The templates are better for showing at the mechanics of making it operational.

@juliettelavoie
Copy link
Contributor

juliettelavoie commented Aug 7, 2023

yes, I will create a new PR

@juliettelavoie
Copy link
Contributor

new PR: #235

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants