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

Add comments with TODOs after LaTeXML issue is fixed #2882

Merged
merged 4 commits into from
Mar 5, 2021

Conversation

henrikt-ma
Copy link
Collaborator

These are notes for the future, after narrowing down what #2867 was really about.

@henrikt-ma
Copy link
Collaborator Author

Now that I understand the mechanisms behind the problems, I realized that it made more sense to put a workaround in place than just wait for the external issues to be fixed.

function equalityConstraint // non-redundant equality
input Type T1;
input Type T2;
output Real residue[ <n> ];
output Real residue[$n$];
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure about skipping angles here (and the similar line below). The n here should normally be replaced by a number (such as 3 for quaternions).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Couldn't we solve this nicely by rephrasing this slightly and placing it before the listing?

The \lstinline!residue! output of the \lstinline!equalityConstraint! function shall have known size, say constant $n$.

Copy link
Member

Choose a reason for hiding this comment

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

I also like having it as <n>.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't find the angle bracket construct very pretty in general, but still useful for placeholders in need of a textual description. For something acting as a placeholder for a number, I find it more consistent to use the rather well established use of "meta variables" typeset as math.

Copy link
Member

Choose a reason for hiding this comment

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

What are the well-established meta-variables? Where are they established?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It strikes me now that the use of n in the listing would actually assume that it is an Integer dimension – is this really the current intention? To me, the current text would actually allow an array indexed by Boolean or enumeration (both of which will result in a constant size, which is the requirement), and I don't see that it would really be a problem to do so. If we want to make it more clear that this is allowed, I suggest this:

Suggested change
output Real residue[$n$];
output Real residue[$\langle$$\mbox{\emph{constant size dimension}}$$\rangle$];

In the text, we could elaborate this further:

The \lstinline!residue! output of the \lstinline!equalityConstraint! function shall have known size, say constant $n$. (That is, $n$ cannot depend on inputs to the function, and the array cannot have flexible size given by colon (\lstinline!:!).)

Copy link
Member

@eshmoylova eshmoylova Mar 4, 2021

Choose a reason for hiding this comment

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

I would be OK with removing the angled brackets around n. But I feel like we need to open a separate issue to clarify what "known size" means. If we come up with a concise wording we could consider putting that into the angle brackets in the code.

Copy link
Member

Choose a reason for hiding this comment

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

Created #2886.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good. @HansOlsson, would you then be OK with the resolution to move (with some rephrasing) the sentence introducing n to before the listing?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would be ok with different variants of that - and I agree with @eshmoylova that it seems best to have a separate issue for that, so resolving, accepting and merging this.

If we'd like to avoid spelling out "colon", we should quote the colon rather than putting it inside parentheses.
@HansOlsson HansOlsson merged commit b349a3a into modelica:master Mar 5, 2021
@henrikt-ma henrikt-ma deleted the comment-mathescape-emph branch March 7, 2021 15:07
@HansOlsson HansOlsson added the M36 For pull requests merged into Modelica 3.6 label Jul 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
M36 For pull requests merged into Modelica 3.6
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants