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 @show_name invocations #1885

Closed
wants to merge 1 commit into from
Closed

Conversation

fingolfin
Copy link
Member

Also update ConstPolyRing show method

@lgoettgens
Copy link
Collaborator

@show_name requires the object to have attribute storage enabled

@fingolfin
Copy link
Member Author

I don't think so? We specifically wrote it to also work for non-attribute storing types.

But @show_special does require it. However I wonder if we should change that, so that one can write show methods w/o having to worry about attribute storage being available or not.

But that's for another PR, if at all. For now, perhaps we don't want matrix spaces to print by name because they are supposed to be uniquely identified by their base ring plus dimensions. So perhaps I'll instead leave a comment there indicating we deliberately don't use show_name and show_special.

@lgoettgens
Copy link
Collaborator

I don't think so? We specifically wrote it to also work for non-attribute storing types.

But @show_special does require it. However I wonder if we should change that, so that one can write show methods w/o having to worry about attribute storage being available or not.

I'll provide a PR that makes show_special a noop in these cases

@fingolfin
Copy link
Member Author

With the ConstPolyRing changes moved to PR #1894 this is now rather small: on the one hand MatRing and MatSpace as mentioned before -- but perhaps we ought to retain those as they are?
And then for NewRing in the tests. That show method is specifically for testing the three printing modes, so perhaps it should also stay as it is.

So in principle I'd be OK with closing this...

@fingolfin fingolfin closed this Nov 12, 2024
@fingolfin fingolfin deleted the mh/show_name branch November 12, 2024 17:14
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.

2 participants