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

Fix: show rate expression instead of rate name #1756

Merged
merged 15 commits into from
Sep 8, 2023

Conversation

blanchco
Copy link
Contributor

@blanchco blanchco commented Aug 23, 2023

Description

  • Show rate expression instead of rate name
  • Did some post processing on the transition in the ACset before we send it to the acset-to-latex endpoint.
  • expressions will show within brackets

Screenshot 2023-08-23 at 2 06 13 PM

Screenshot 2023-08-23 at 2 02 00 PM

Resolves #(issue)

@blanchco blanchco self-assigned this Aug 23, 2023
@blanchco blanchco linked an issue Aug 23, 2023 that may be closed by this pull request
@blanchco blanchco marked this pull request as ready for review August 23, 2023 18:20
@mwdchang
Copy link
Member

So I noticed there is a slight discrepancy here, for example we have

  • susceptible, versus
  • susceptible(t)

Is this something we should address @liunelson

@liunelson
Copy link
Member

liunelson commented Aug 23, 2023

I have a few comments:

  1. Why are we still using an ACSet-to-LaTeX converter, as opposed to an AMR-to-LaTeX converter? It took me a little to even find a ACSet to even test that model-service endpoint!
  2. (t) in the returned LaTeX should probably be always not shown since it is effectively redundant. The fact that an entity has a differential equation d X / d t = f(X) associated with it is sufficient to determine that it is time-dependent. Furthermore, I tested TA1's equation-to-AMR converter and found that it strips the (t) anyway. Showing the (t) would make the user type more LaTeX than necessary.
  3. It is becoming increasingly untenable to permit parameters and variables to have IDs that are words (i.e. multi-character strings). Justin (developer for the TA1 equation-to-AMR converter) says that we should be only allowing IDs of the form X_{abc} (not Xabc, abc, X_abc, etc.). Else, we risk interconverting AMRs and equations that interpret a variable/parameter ID such as poParam into the product of p, o, P, a, r, a, m.
  4. It is somewhat mentally taxing to read rendered equations and map them to un-rendered ones in the graph. Ideally, the equations in the graph should be rendered as well. Same goes with the labels of the state variables.
  5. Generally, when typesetting math equations, multiplication is implicit - i.e. no *, maybe a non-breaking space. The asterisk or star symbol * is usually reserved for special operators like convolution or complex conjugation. If we really want to show multiplication, the standard is to use \cdot (i.e. ·) symbol.

@blanchco
Copy link
Contributor Author

blanchco commented Sep 7, 2023

Updates:

  • Now we can no longer edit equations when a model is already present as per the outcome of the discussion
Screen.Recording.2023-09-07.at.3.01.10.PM.mov

Screenshot 2023-09-07 at 3 01 40 PM

@blanchco blanchco changed the title Fix: show rate eexpression instead of rate name Fix: show rate expression instead of rate name Sep 7, 2023
@blanchco blanchco requested a review from mwdchang September 7, 2023 19:24
@blanchco blanchco requested a review from YohannParis September 8, 2023 13:40
Co-authored-by: Yohann Paris <github@yohannparis.com>
@blanchco blanchco merged commit defbef8 into main Sep 8, 2023
@blanchco blanchco deleted the fix-show-rate-eexpression-instead-of-rate-name branch September 8, 2023 19:24
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.

[BUG]: Equations should not have transition names in them
4 participants