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

Introduce preference for automatic ascii visualization of system #122

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

Conversation

mfherbst
Copy link
Member

Follow-up to #121 to discuss ascii visualisation preferences. Ping @cortner @rkurchin @jgreener64 to get some opinions.

I personally find it quite convenient to automatically get this insight when working in the REPL or a notebook, but this may just be me. I also found it helps students ... but again I introduced it, so I'm biased.

Options in my opinion are:

  • Don't do it, people have to do it by calling visualize_ascii manually each time. Pros: Clear what happens, least complexity; Cons: It's an extra function to learn for something where Julia has builtin mechanics.
  • Use some preference to configure it, e.g. Just enable/disable, some threshold in terms of number of atoms (done right now) or some threshold in terms of "density" of atoms on the canvas (to avoid cluttered ascii art, which is useless); Pros: Everyone can do as they please, if properly documented with helper functions should be fairly intuitive; Cons: We have to agree on defaults because most people will not change them.

In fact there may even be some builtin show preferences in Julia one can use for the second point ... I vaguely remember that, but I have not checked.

Other thoughts ?

@rkurchin
Copy link
Collaborator

I personally haven't made much use of it, and have occasionally found it distracting, but can also imagine cases where it would be useful as you say. I would be fine with either option you describe.

@jgreener64
Copy link
Collaborator

I don't view systems enough in the REPL to have strong opinions on this.

@cortner
Copy link
Member

cortner commented Oct 23, 2024

I'm ok with either option. If we go with a settable default then I will obviously vote to have a slim display as the default.

@mfherbst
Copy link
Member Author

Ok, I added the preferences. By default visualize_ascii will not be called.

@mfherbst
Copy link
Member Author

If one of you gives your ok, I guess we can merge.

Copy link
Collaborator

@rkurchin rkurchin left a comment

Choose a reason for hiding this comment

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

Generally LGTM, should for sure fix the one tiny typo, and if people want to discuss the minor potential ambiguity, we can, but I'm also okay just merging this as-is since it's not a huge deal

src/utils/show.jl Outdated Show resolved Hide resolved
src/utils/show.jl Outdated Show resolved Hide resolved
@rkurchin
Copy link
Collaborator

Is this basically good to go? I don't think it should clash with #130 AFAICT, so presumably once coverage issues are addressed, it can be merged? I might have some time to add those couple of tests this afternoon if you don't already have them locally or something...

@mfherbst
Copy link
Member Author

No go ahead !

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.

4 participants