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

basic implementation of DisplayForm #604

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

basic implementation of DisplayForm #604

wants to merge 8 commits into from

Conversation

mmatera
Copy link
Contributor

@mmatera mmatera commented Nov 10, 2022

This PR provides a basic / preliminar implementation for DisplayForm.

Copy link
Contributor

@TiagoCavalcante TiagoCavalcante left a comment

Choose a reason for hiding this comment

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

LGTM

CHANGES.rst Show resolved Hide resolved
in_outputforms = True
summary_text = "render low-level box structures"

def apply_makeboxes(self, expr, f, evaluation):
Copy link
Member

Choose a reason for hiding this comment

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

apply -> eval in new code please.

@@ -394,6 +394,10 @@ def apply_general(self, expr, f, evaluation):
"""MakeBoxes[expr_,
f:TraditionalForm|StandardForm|OutputForm|InputForm|FullForm]"""
if isinstance(expr, BoxElementMixin):
# If we are inside a DisplayForm block,
# BoxElementMixin are not processed.
if evaluation.in_display_form:
Copy link
Member

@rocky rocky Nov 13, 2022

Choose a reason for hiding this comment

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

Can this be handled by some sort of HoldForm attribute?

I know DisplayForm doesn't have the HoldForm attribute, but if the attribute mechanism were hooked into somehow we wouldn't need to add a special flag and test.

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 don't think so. In the first place, I guess that in WMA , it does not have that attribute for a reason. But, I also tried to avoid the new attribute, and I could not figure out how to do that without changing more parts of 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.

Ok - I'd like to check on this. Doing this kind of thing feels weird.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking about it again, one alternative would be to set a symbol in a private context, and then instead of looking at that property, look at the value of the symbol. This could have the advantage to make easy to replace the Python code that implements MakeBoxes by WL code. But in the end, it would be the same idea.

Copy link
Member

@rocky rocky Nov 13, 2022

Choose a reason for hiding this comment

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

To start out, I believe we should be following the way that can be written in WL. This keeps us from inventing mechanisms, and flags and so on. Also, since WL follows a certain logic, makes it more likely we will go with the natural flow here, without having to consider specially edge cases. They are probably the ones that WL has set up.

If later we need to get things faster then starting with the logical flow is better than some guessed version.

Copy link
Member

@rocky rocky Nov 15, 2022

Choose a reason for hiding this comment

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

As I think I mentioned on the other PR, TagBox might be used by DisplayForm and that is probably a mechanism by which we get special behavior.

See https://reference.wolfram.com/language/ref/TraditionalForm.html

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.

3 participants