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.

Sign up for GitHub

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

Jump to bottom

Various docs fixes #592

Merged
merged 42 commits into from
Feb 16, 2023
Merged

Various docs fixes #592

merged 42 commits into from
Feb 16, 2023

Conversation

TorkelE
Copy link
Member

@TorkelE TorkelE commented Feb 7, 2023

Not planning to make any major changes in the PR, just various misc fixes.

@TorkelE
Copy link
Member Author

TorkelE commented Feb 7, 2023

The sub headers where different across the docs (using ## or ##, capitalising every word or just the firs).

Currently, I set it to:
Each doc page start with a # header, followed by ## headers.
In the first title on each page every word is capitalised (although wouldn't mind changing to only the firs). Following headers, only the first word is capitalized.

@TorkelE
Copy link
Member Author

TorkelE commented Feb 7, 2023

I added two more models to the basic CRN examples as that one felt a bit thin. I also set it so that each of those examples begins with

using Catalyst, DifferentialEquations, Plots

(rather than just the first one)

That one someone can copy whichever example they want and run it directly without adding the using ...
(happy to revert this if you prefer the old version)

isaacsas
isaacsas reviewed Feb 8, 2023
View reviewed changes
docs/src/catalyst_functionality/network_analysis.md Outdated
@@ -235,7 +235,7 @@ complexgraph(rn)

![network_1](../assets/complex_rn.svg)

### Linkage classes and sub-networks of the reaction network
## Linkage classes and sub-networks of the reaction network
Copy link
Member

Choose a reason for hiding this comment

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

This was intentionally meant to be a sub-subsection.

docs/src/catalyst_functionality/network_analysis.md Outdated
@@ -274,7 +274,7 @@ and,

![subnetwork_2](../assets/complex_subnets2.svg)

### Deficiency of the network
## Deficiency of the network
Copy link
Member

Choose a reason for hiding this comment

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

This was intentionally meant to be a sub-subsection.

docs/src/catalyst_functionality/network_analysis.md Outdated
@@ -326,7 +326,7 @@ Quoting Feinberg [^1]
> stoichiometry vectors] are as independent as the partition of complexes into
> linkage classes will allow.

### Reversibility of the network
## Reversibility of the network
Copy link
Member

Choose a reason for hiding this comment

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

This was intentionally meant to be a sub-subsection.

docs/src/catalyst_functionality/network_analysis.md Outdated
@@ -375,7 +375,7 @@ isweaklyreversible(rn, subnets)
Every reversible network is also weakly reversible, but not every weakly
reversible network is reversible.

### Deficiency Zero Theorem
## Deficiency Zero Theorem
Copy link
Member

Choose a reason for hiding this comment

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

This was intentionally meant to be a sub-subsection.

@isaacsas
Copy link
Member

isaacsas commented Feb 8, 2023

Aside from the sub-subsection comments LGTM.

@TorkelE
Copy link
Member Author

TorkelE commented Feb 8, 2023

What do you think of making the ### headers into #### headers? Right now it is slightly hard to distinguish the ### ones from the ## ones. If we make them #### it might be clearer that these are subsections?
(and I don't think we will ever subsection something using all three of ##, ###, and ####, so might as well use the edge cases)

@isaacsas
Copy link
Member

isaacsas commented Feb 8, 2023

Sure, as long as it keeps them larger than the surrounding text.

@TorkelE
Copy link
Member Author

TorkelE commented Feb 10, 2023

Yep it does.

(this is in fact what my hack solution does)

Not sure why the new latexify fails in the docs, ti works locally.

It would probably be good to understand this, but would probably need to talk to Niklas on what is actually going on in latexify, and I am not sure how much time he has for that package these days. Alternatively, we can add this as an issue and go with this/revert latexify changes.

@isaacsas
Copy link
Member

If they print correctly after conversion in the docs just change to that instead, and drop showing off the shorthand version for now (since it isn't really working consistently).

@TorkelE
Copy link
Member Author

TorkelE commented Feb 10, 2023

I guess we could do that. But don't you think the latexify(rn; form=:ode) should be the main approach? It seems weird to use an alternative. Also, it seems to be working in non-documenter situations. I could try to dig in to what is going on.

@TorkelE
Copy link
Member Author

TorkelE commented Feb 10, 2023

I've redone it using convert, and will raise an issue.

@TorkelE TorkelE mentioned this pull request Feb 10, 2023
Closed
@isaacsas
Copy link
Member

Sounds good. We can switch back to your form once it is working.

@TorkelE
Copy link
Member Author

TorkelE commented Feb 10, 2023

Bot this and #591 seems to work now, should I try to merge them?

@isaacsas
Copy link
Member

I need to look this one over again, I’m still seeing new typos.

@TorkelE
Copy link
Member Author

TorkelE commented Feb 10, 2023

Sounds good, I will fix them as you find them. Thanks for having a proper look at it.

isaacsas
isaacsas requested changes Feb 11, 2023
View reviewed changes
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
docs/src/catalyst_applications/bifurcation_diagrams.md Outdated Show resolved Hide resolved
docs/src/catalyst_functionality/compositional_modeling.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
docs/src/catalyst_functionality/network_analysis.md Outdated Show resolved Hide resolved
docs/src/catalyst_functionality/parametric_stoichiometry.md Outdated Show resolved Hide resolved
docs/src/catalyst_functionality/parametric_stoichiometry.md Outdated Show resolved Hide resolved
docs/src/catalyst_functionality/programmatic_CRN_construction.md Outdated Show resolved Hide resolved
docs/src/faqs.md Outdated Show resolved Hide resolved
TorkelE and others added 8 commits February 10, 2023 22:21
@TorkelE @isaacsas
Update README.md
ae18ee9 
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
@TorkelE @isaacsas
Update docs/src/catalyst_functionality/dsl_description.md
4803d66 
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
@TorkelE @isaacsas
Update docs/src/catalyst_functionality/dsl_description.md
b5a73df 
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
@TorkelE @isaacsas
Update docs/src/catalyst_functionality/parametric_stoichiometry.md
47aef5c 
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
@TorkelE @isaacsas
Update docs/src/catalyst_functionality/parametric_stoichiometry.md
c90cd75 
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
@TorkelE @isaacsas
Update docs/src/faqs.md
7fbba9d 
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
@TorkelE @isaacsas
Update docs/src/catalyst_functionality/programmatic_CRN_construction.md
20c980b 
Co-authored-by: Sam Isaacson <isaacsas@users.noreply.github.com>
@TorkelE
fixes
2435e2b
@TorkelE
Copy link
Member Author

TorkelE commented Feb 11, 2023

Ok, fixed after your comments, in summary, what is left hanging:

  • One case where I rephrased the sentence, if you want we can rephrase that though.
  • Whenever we want to move to 4 spaces throughout the docs, I am for this, but had some memory of there was a reason for 2 spaces.
  • I'm ok to use latexif(convert(ODESystem, rn)) for now, but when it works I think we should move to latexify(rn; form=:ode) in most places. But unless we want to we do not need to settle that until that is actually possible.

@isaacsas
Copy link
Member

This LGTM.

@isaacsas
Copy link
Member

If tests pass and the doc build looks ok this should be GTG.

@ChrisRackauckas
Copy link
Member

Just delete the unused SVGs.

@isaacsas isaacsas merged commit e2dfcd0 into master Feb 16, 2023
@isaacsas isaacsas deleted the minor_doc_changes branch February 16, 2023 19:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@isaacsas isaacsas isaacsas requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@TorkelE @isaacsas @ChrisRackauckas