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

t-route (master branch) docs update #506

Merged
merged 2 commits into from
Jun 5, 2023

Conversation

mattw-nws
Copy link
Contributor

Documentation update to go with #502

Additions

  • Information specific to master branch, building and config differences specific to ngen

Removals

  • Old info relevant only to previous t-route (ngen branch) version
  • References to using extern/t-route, which is no longer recommended

Changes

  • Removed some content and pointed back to t-route's page instead... this de-scoped documentation really needs to belong in the t-route repo and not in ngen, what is here is now largely specific to running t-route in/with ngen.

Testing

Screenshots

Notes

Todos

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows project standards (link if applicable)
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future todos are captured in comments
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
  • Reviewers requested with the Reviewers tool ➡️

Testing checklist (automated report can be put here)

Target Environment support

  • Linux

@mattw-nws mattw-nws force-pushed the t-route-master-docs branch from ab36ce3 to 24633b9 Compare April 11, 2023 19:20
@aaraney aaraney self-assigned this May 26, 2023
Copy link
Member

@aaraney aaraney left a comment

Choose a reason for hiding this comment

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

Thanks for adding this @mattw-nws! Ive added a few minor edits that im happy to contribute to your branch if you would like me to do that?

doc/PYTHON_ROUTING.md Outdated Show resolved Hide resolved
doc/PYTHON_ROUTING.md Outdated Show resolved Hide resolved
doc/PYTHON_ROUTING.md Outdated Show resolved Hide resolved

The t-route extension modules rely on netcdf fortran, and thus they need to be compiled with the same fortran compiler that compiled
Copy link
Member

Choose a reason for hiding this comment

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

I would add "Note," to begin this paragraph as it does not affect all installations.

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've sometimes abused > for this kind of thing (blockquote), do you happen to know some GHMD-fu for this use-case?

Copy link
Member

Choose a reason for hiding this comment

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

I often bold NOTE at the beginning of the line, but that also not great. Im not aware of a more semantic way. I like you block quote approach though. The appearance is more like a text book or blog post.

doc/PYTHON_ROUTING.md Outdated Show resolved Hide resolved

To enable routing in a simulation realization config file, a `routing` block should appear with a path to the t-route configuration file at the same level as the `time` configuration, like so:

```JSON
Copy link
Member

Choose a reason for hiding this comment

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

Not crucial, but you can use "json5" as the block language type instead of json which enables comment characters. This will fix the formatting of ....

// comment
// ...
[1, 2, 3]
// ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Neat... I have sometimes used Javascript instead but folks complained.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

of course.... ngen's JSON parser doesn't support comments... yet... so that might lead to consternation for some.

Copy link
Member

Choose a reason for hiding this comment

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

That's a great point. Ill leave it as is!

Copy link
Member

Choose a reason for hiding this comment

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

Also my Friday is now instantly better having read the work consternation. Excellent word choice!

@mattw-nws
Copy link
Contributor Author

Thanks for adding this @mattw-nws! Ive added a few minor edits that im happy to contribute to your branch if you would like me to do that?

Please feel free

@aaraney
Copy link
Member

aaraney commented Jun 2, 2023

Thanks for adding this @mattw-nws! Ive added a few minor edits that im happy to contribute to your branch if you would like me to do that?

Please feel free

Sorry, @mattw-nws, for whatever reason I didnt get a notification that you'd responded to this PR review. Sorry i've let it go a little dormant! Ill make the changes and push them to your branch now.

@aaraney
Copy link
Member

aaraney commented Jun 2, 2023

@mattw-nws, just made the changes to your branch. If you are happy with the changes, I think we are good to merge.

@aaraney aaraney merged commit 2a8fcf2 into NOAA-OWP:master Jun 5, 2023
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.

2 participants