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

Minor fixes to code & edits of docs +++ textual feedback #45

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

Conversation

mherrmann3
Copy link

@mherrmann3 mherrmann3 commented Oct 18, 2024

I went through the docs and executed the tutorials; I really enjoyed it. The only critical fix to make models run on my system is commit 'Run models whose absolute path contains a whitespace'. The rest are purely suggestions. @pabloitu Feel free to take what you need 😏

Further comments that require more elaboration follow below 🙂


Note: comments starting with 'PR:' are already addressed in this PR; for the other ones I did not yet provide code suggestions either because they would be too difficult/complex for me or because you have to decide by yourself how you'd address it)

  • I installed floatCSEP both via the latest version from github and the stable release from conde-forge; both install fine.

    • but strange: the latest version from github is at v0.1.5 (when installed with pip), the one from conda-forge at v0.2.0. Not sure why, even though both end at commit df2dbe0. I expected the opposite or at least both on v0.2.0.
      btw: floatcsep/__init__.py is at __version__ = "0.1.4"
  • installation instructions:

    • PR (commit): conda env create -n csep_env expects an environment.yml. So this can only be executed inside the floatCSEP folder; what you meant (in both cases) is conda create -n csep_env (without env). To keep using the one with env at least for section Latest Version > 1. Using conda, you only have to reorder your commands, that is, create env after cd floatcsep; this has a bonus: all dependencies are installed by conda, not pip. I did that in my PR and provided further suggestions. I guess this solves the issue of Toño.
    • the README section "Installing floatCSEP" is not in line with the docs (for instance, no need to install pycsep manually, since it's already specified as dependency); I'd only mention the conda-forge install option and link to the docs for alternative ways.
  • markdown reports:

    • PR (commit): I'd output relative instead of absolute links to figures; in this way the whole results folder becomes "portable" and self-contained, that is, can be moved somewhere else or to other devices without breaking links.
    • to consume reports without a markdown-compatible editor, also generate a .html file, which can be viewed with a browser; either write out directly or convert from markdown using a separate package like markdown or pygments. However, those won't be consumed by GitHub.
    • to avoid what Francesco observed when viewing the markdown report in a wide window, specify a reasonable width= parameter for the 'Input catalog' figures (I like 800). (I didn't touch the code because I didn't know how to do it for this figure table.)
    • PR (commit): the test results should get their own ##-heading, otherwise they hierarchically appear under "Authoritative Data".
  • plots/figures:

    • the 'catalog' figure: it has super-mini axis labels and excessive whitespace, which creates large margins in the report.
    • the 'Sequential *' plots: for better reference, the relative year in the x-axis label (0, 1, ..) might be converted into a corresponding date axis (2010, 2011, ...).
  • Case D:

    • now that the used forecasts become more interesting, perhaps mention that they are plotted in 2020-01-01_2021-01-01/figures (together with the test results). Also, it might we worthwhile to include them in the report (e.g., a section between input data and results: ## Forecast(s)).
  • Case E:

    • Setting use_db: False is indeed muuuch slower (~10x).
      • Q1: Why not setting use_db: True as the default in init of TimeIndependentModel?
      • Q2: The HDF5Serializer will eventually be replaced by dbcsep, correct?
    • Post-process & Plot command sections: the forecast plots are not in {timewindow}/forecasts but (only) in 2010-01-01_2022-01-01/figures (i.e., only the last date and together with the test results). Again, it might we worthwhile to include them in the report.
    • Plot command section: actually, the provided colormap was already used in the config.xml. I think you wanted to specify a different one, e.g., 'viridis'?
    • Q: I spotted a case_e.py file in the folder; does it have a purpose?
  • Case F:

    • test results in the report only refer to the last date
      • Q: being T-D, perhaps create a report for each date? Otherwise, mention this aspect.
    • likewise and as before, the forecast is only plotted for the last date (2016-11-20_2016-11-21/figures). This made sense for time-independent models, but not so much for a T-D forecast. Also, it might we worthwhile to mention those outputs again in the doc of case F (and perhaps include them in the report).
  • Case G:

    • I believe using build: venv is incompatible with running floatCSEP in a conda env; this warning is printed and it doesn't find pymock. In the following, I use build: conda (using mamba, which it nicely detects) and assume that build: venv won't work inside an active conda env; not sure if it works on your system.
      • you may simply omit build: venv to use conda by default (since most users will work in a conda env per install instructions); if you do, don't forget to adapt the 'Important' box;
      • perhaps mention that this conda environment must be removed again manually if not needed anymore (and how: conda remove -n pymock_<hash> --all);
      • in the 'Important' box, mention that build: venv won't work when already working in a conda environment (if it is truly so).
    • PR (commit): if an experiment (or pymock) has an absolute path that contains a whitespace, nothing will run; it is important to put the args_file path into quotation marks (see commit)
      • btw: with build: venv, additionally also the activate_script path must be set in quotation marks (see commit)
    • Regarding custom plots, it would be nice to automatically include them into the report in a separate section below the test results (## Custom plots).
    • same as before:
      • mention that the test results in the report only refer to the last date (and perhaps create a report for each date?)
      • the forecast is only plotted for the last date (2012-06-13_2012-06-20/figures) + it might we worthwhile to mention those outputs again in the doc of case G (and perhaps include them in the report).
  • Case H:

    • Like Toño & Francesco, I also have problems installing the etas model. conda seems to error on installing the pynverse package. Perhaps the newer v0.1.4.6 fixes it?
  • misc:

    • PR (commit) I allowed myself to make minor wording and typographic edits in tutorials E,F,G,H (+ provide & link some references).
    • I found floatcsep, floatCSEP, and floatCSEP in the docs; I'd homogenize it for "brand recognition" 😄
      • same for the logo: different in the README and docs

(instead of absolute ones)
... in this way the whole `results` folder becomes "portable" and self-contained, that is, it can be moved somewhere else or to other devices without breaking links.
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.

1 participant