Make HistogramVisual updatable after instantiation

[tlambert03]

adds set_raw_data method to HistogramVisual to allow data to be updated after the initial instantiation. Also allows the user to customize the method used to calculate the histogram (defaults to numpy.histogram)

[djhoese]

Oh and does this need tests?

[tlambert03]

thanks for the comments @djhoese! Updated the typing and docs. haven't added a test yet. Technically, it should have the same coverage as before, since it's mostly just refactoring existing functionality into separate methods. but I'll have a look at the existing tests and see if I can extend them easily

[tlambert03]

ok, updated the test here to instantiate without data, and use the set_raw_data method to update the plot.

[djhoese]

Thoughts on removing the type definitions in the docstring? We'd have to check the generated docs, but I'm pretty sure sphinx should handle this fine and include the types in the API docs. Not sure it is needed in the docstring otherwise, right?

[tlambert03]

i don't have any strong thoughts on that. I'd say it's a project-specific decision, and I can see arguments for and against. but, I'm happy to do whatever you want! if vispy is moving in a particular direction with docstrings and you want to use this PR to update this particular module, that's fine, just let me know how you want it to look :)

[djhoese]

@brisvag Thoughts? @tlambert03 what have you seen other projects do? It seems redundant to have you put this much work into defining the type annotations with TypeAliases and then have to type them again in the docstrings. Could you try removing them and generating the docs locally? Or if you want, push here and we can download the website artifact from CI and view it.

cd doc
make html_dev-pattern PATTERN=abcd

^ this should generate the docs but skip all the examples.

[tlambert03]

I agree it's redundant. but I do still think it's slightly a matter of opinion. I think you'll find some holdouts who feel like docstrings are for humans and type annotations are for machines, and that docstrings are "sacred" in a sense... people in that camp would probably say "go ahead and use type annotations if you want, but leave my docstrings out of it". They might additionally argue that strictly accurate type annotations can become verbose at the expense of human readability. then there's strict DRY camp that says it should only ever be in one place, even if it possibly requires someone to look in a different place than they might be accustomed to in interactive usage. Or those who simply feel that type annotations are more standardized than what people tend to put in their docstrings...

(both might argue that they don't care how your rendered HTML docs look, or whether you're using sphinx type hints to achieve the same final result on your webpage)

i kinda like the modern type-hints only approach... but more than anything this feels like a repo-wide discussion, and not something I would try to change incrementally in each PR that happens to touch a docstring. I added the "regular" docs to match the rest of vispy, I added the type hints after your comment suggested to add them ... I think this general discussion belongs somewhere outside of this PR :)

[djhoese]

Sure. I think my thing is that I typically either am looking at API docs on the website or I'm telling my IDE to pull them up for me. I rarely open the docstring version of the docs in my IDE and only pull up the "show me the parameters and their types" view inline as I'm typing. I think PyCharm is smart enough to put type annotations where it needs to in these cases. Anyway...I don't think vispy is maintained well enough to do this repo-wide so I personally would be OK with it being done here as a test...assuming it renders properly in sphinx.

[djhoese]

@tlambert03 Maybe that leaves it up to you then since we're asking you to do the work. I think I'd prefer type annotations only, @brisvag seems to have a slight preference for docstrings (human-readable). I don't think NOT having type annotations is a reasonable option so the choice is both or annotations only. Again, I vote for annotations only and we see how it goes with user complaints (if any).

[tlambert03]

ok, so just so I'm clear. you want me to add type annotations to parts of the code didn't previously have them? (like that one method in plotwidget)? You're ok with just that single method having type annotations? or you want me to add it to the whole module? (this is why i find this all a bit confusing... i imagined this would be a quick PR that just moves a little existing code around to allow it to be called as a method 😄 )

[brisvag]

Then it looks like what we have here already will do just fine (better than anything we had before anyways :P)

[djhoese]

My thought is since you added type annotations for every argument in the __init__ that it made sense to remove them from the docstring for __init__/the class. This is inline with my opinion that docstring types aren't necessary if type annotations are there.

BUT I don't think other future PRs where someone adds a single new kwarg and puts a type annotation on it means that they should remove types from the docstrings. I think it is an all-or-nothing kind of thing on a per-method basis (maybe a per class).

So I think I'd prefer removing the docstring types for any documented method in this file that you already modified and added type annotations for. If it doesn't have type annotations or you didn't modify it then feel free to leave it as-is. I see this as more of an experiment than a complete cleanup.

[tlambert03]

ok, will do. I believe I added type annotations in the __init__ in response to a previous comment ... it wasn't in the original PR, which largely aimed to match the existing code patterns.
I'll go ahead and remove docstring types in HistogramVisual

[brisvag]

Nice!

[brisvag]

Then it looks like what we have here already will do just fine (better than anything we had before anyways :P)

[djhoese]

Looks pretty good:

[djhoese]

Wait...those docstrings don't match the current state of your PR. WTF

Edit: Side note, you need two backticks around literals to go from italics to literal code block. Like:

``None``

[djhoese]

I'm going to rerun the documentation building job to see if the docstrings make more sense. Otherwise, @tlambert03 if you have time you could generate the docs locally and see if the API docs docstrings match for you.

[djhoese]

@tlambert03 I seem to not have push permissions on your fork. Any idea why?

[tlambert03]

hmm, I'm not sure. I do have the box checked to allow edits by maintainers, and it is from a branch in my personal fork... but I have had this happen before in PRs to vispy... so I wonder if there's something wrong about my fork. sorry about that. will try to build docs locally

[djhoese]

I did end up making a PR in your fork. If you merge that there should be some better handling of type annotations by sphinx.

[djhoese]

Did you change something? I was able to make a commit now. This time I did it in github's basic editor, before I was doing it in a "github codespace".

[djhoese]

Ok so issues:

1. The building of the website failing doesn't cause the CI to fail. Even if it doesn't produce anything the artifact upload doesn't even fail.
2. Error from a recently merged PR:

/home/runner/micromamba/envs/vispy-tests/lib/python3.9/site-packages/numpydoc/docscrape.py:455: UserWarning: Unknown section Parameters: in the docstring of assert_image_reasonable in /home/runner/work/vispy/vispy/vispy/testing/image_tester.py.
  warn(msg)

3. Error from this PR:

WARNING: error while formatting signature for vispy.scene.visuals.Histogram: Handler <function mangle_signature at 0x7f3f90cad040> for event 'autodoc-process-signature' threw an exception (exception: The section Parameters appears twice in  Visual that calculates and displays a histogram of data

    This class inherits from visuals.HistogramVisual and scene.Node, allowing the visual to be placed inside a scenegraph.
looking for now-outdated files... none found


    Parameters
    ----------
    data
        Data to histogram.  May be `None` on initialization, use `set_raw_data`
        to set data after initialization.
    bins
        If `bins` is an int, it defines the number of equal-width
        bins in the given range (10, by default). If `bins` is a
        sequence, it defines a monotonically increasing array of bin edges,
        including the rightmost edge, allowing for non-uniform bin widths.
        May also be a string if the calc_hist function supports it.
    color
        Color of the faces in the histogram mesh.
    orientation
        Orientation of the histogram.
    calc_hist
        Function that computes the histogram. Must accept two positional arguments
        (data, bins) and return (hist_data, bin_edges). Default is numpy.histogram.
    **kwargs
        Keyword arguments to pass to `MeshVisual`.

    parent : Node
        The parent node to assign to this node (optional).
    name : string
        A name for this node, used primarily for debugging
        (optional).
Visual that calculates and displays a histogram of data

    Parameters
    ----------
    data
        Data to histogram.  May be `None` on initialization, use `set_raw_data`
        to set data after initialization.
    bins
        If `bins` is an int, it defines the number of equal-width
        bins in the given range (10, by default). If `bins` is a
        sequence, it defines a monotonically increasing array of bin edges,
        including the rightmost edge, allowing for non-uniform bin widths.
        May also be a string if the calc_hist function supports it.
    color
        Color of the faces in the histogram mesh.
    orientation
        Orientation of the histogram.
    calc_hist
        Function that computes the histogram. Must accept two positional arguments
        (data, bins) and return (hist_data, bin_edges). Default is numpy.histogram.
    **kwargs
        Keyword arguments to pass to `MeshVisual`. in the docstring of Histogram in /home/runner/work/vispy/vispy/vispy/scene/visuals.py.)
WARNING: Failed guarded type import with ImportError("cannot import name 'TypeAlias' from 'typing' (/home/runner/micromamba/envs/vispy-tests/lib/python3.9/typing.py)")
WARNING: Cannot resolve forward reference in type annotations of "vispy.visuals.histogram.HistogramVisual": name 'npt' is not defined
WARNING: Cannot resolve forward reference in type annotations of "vispy.visuals.histogram.HistogramVisual.orientation": name 'Orientation' is not defined
WARNING: Cannot resolve forward reference in type annotations of "vispy.visuals.histogram.HistogramVisual.set_raw_data": name 'npt' is not defined

My understanding is that the create_visual_node function that creates a SceneCanvas Visual object from the basic Canvas Visual class is concatenating the docstrings in some way that is producing two sets of "Parameters" sections. I'm a little worried this is going to be annoying to fix.

[djhoese]

vispy/vispy/scene/visuals.py

Lines 146 to 226 in 6667599

	def generate_docstring(subclass, clsname):
	# Generate a Visual+Node docstring by modifying the Visual's docstring
	# to include information about Node inheritance and extra init args.
	sc_doc = subclass.__doc__
	if sc_doc is None:
	sc_doc = ""
	# find locations within docstring to insert new parameters
	lines = sc_doc.split("\n")
	# discard blank lines at start
	while lines and lines[0].strip() == '':
	lines.pop(0)
	i = 0
	params_started = False
	param_indent = None
	first_blank = None
	param_end = None
	while i < len(lines):
	line = lines[i]
	# ignore blank lines and '------' lines
	if re.search(r'\w', line):
	indent = len(line) - len(line.lstrip())
	# If Params section has already started, check for end of params
	# (that is where we will insert new params)
	if params_started:
	if indent < param_indent:
	break
	elif indent == param_indent:
	# might be end of parameters block..
	if re.match(r'\s*[a-zA-Z0-9_]+\s*:\s*\S+', line) is None:
	break
	param_end = i + 1
	# Check for beginning of params section
	elif re.match(r'\s*Parameters\s*', line):
	params_started = True
	param_indent = indent
	if first_blank is None:
	first_blank = i
	# Check for first blank line
	# (this is where the Node inheritance description will be
	# inserted)
	elif first_blank is None and line.strip() == '':
	first_blank = i
	i += 1
	if i == len(lines) and param_end is None:
	# reached end of docstring; insert here
	param_end = i
	# If original docstring has no params heading, we need to generate it.
	if not params_started:
	lines.extend(["", " Parameters", " ----------"])
	param_end = len(lines)
	if first_blank is None:
	first_blank = param_end - 3
	params_started = True
	# build class and parameter description strings
	class_desc = ("\n This class inherits from visuals.%sVisual and "
	"scene.Node, allowing the visual to be placed inside a "
	"scenegraph.\n" % (clsname))
	parm_doc = (" parent : Node\n"
	" The parent node to assign to this node (optional).\n"
	" name : string\n"
	" A name for this node, used primarily for debugging\n"
	" (optional).")
	# assemble all docstring parts
	lines = (lines[:first_blank] +
	[class_desc] +
	lines[first_blank:param_end] +
	[parm_doc] +
	lines[param_end:])
	doc = '\n'.join(lines)
	return doc

[tlambert03]

Can we go back to my original PR that simply moves the init logo into methods, using the existing patterns, and consider possible changes to how vispy build the docs and how type hints play a role in another issue?

[djhoese]

Sure. Could you please copy this existing branch to a new name and make a new PR and describe it as adding the ability to use type annotations in sphinx docs or something like that? I'd like to not lose what's here already. I suppose the best solution here would be to add the typing back into the docstring and then in doc/conf.py comment out the type annotations extension. Right?