feat: return mappable to allow colorbar

[maximelucas]

Trying to solve #437. So far, only tests.

[maximelucas]

Following what I described in #437 , I have rewritten xgi.draw_nodes() to use plt.scatter()'s full capabilities.

In particular:

1. I'm not using _color_arg_to_dict or _scalar_arg_to_dict anymore. All the scaling happens in plt.scatter().
2. Dict is not anymore an accepted format for node_fc and other similar arguments . Everything is now internally happening using arrays. I don't think we loose much with this.
3. There is no node_ec_cmap anymore. node_ec can only be specified as a color or a sequence of color (no NodeStat or floats). This comes directly from what's allowed in plt.scatter. I don't think we loose much with this.
4. node_fc_cmap is now an explicit argument (and cmaps can be specified as strings now, e.g. "Reds").
5. default params for min/max node size and lw actually work now, soling min/max_val not working in _scalar_arg_to_dict() #442 and default settings in draw() ignored if one of them is passed as argument #443 for that function.

This implementation has the following advantages:

• we get easy colormaps
• the implementation is simpler
• we can easily add even more arguments in the future, that are built in scatter: norm, node_shape (as marker), alpha

Question before I fix tests: are you okay with this?
If yes, should xgi.draw_nodes() return (ax, collection) or just collection?

If we agree on this, we could later look into xgi.draw_hyperedges() to see if we can do something similar.

[nwlandry]

Okay some initial thoughts:

• I really like the thought process behind this and trying to make everything consistent.
• I also like the idea of bringing the API more into line with the matplotlib interface.
• Why do we have to eliminate the dictionary option? Can't we just convert dicts to numpy arrays? We could put this functionality in the _scalar_arg_to_dict _color_arg_to_dict functions and simplify them. I do get your point thought which, if I understand correctly is that (a) the settings should get respected and (b) that scatter() should handle all the coloring and sizing. This does leave me with a few questions:
• Will it be too complicated for users to specify values for the node size if they might have to change the default settings? For example, I think that the values for s that scatter accepts are pixel values and aren't subject to max/min values. From the code, it looks like the values do not get resized to the min/max range specified, but are clipped. This might be unexpected behavior if we aren't careful.

Other issues that are potentially related to this are #174, #272, #280, and #410.

[maximelucas]

• Why do we have to eliminate the dictionary option? Can't we just convert dicts to numpy arrays?

Sure we could do that. I just wasn't sure if it would be a common use case?

We could put this functionality in the _scalar_arg_to_dict _color_arg_to_dict functions and simplify them.

Yes, I thought we might put some of the new code back into simplified versions of those. It was easier to start from scratch at first to know what I really needed. I would probably do that in another PR where we also deal with drawing hyperedges, because if I change _scalar_arg_to_dict it will be break them unless we adapt those too.

• Will it be too complicated for users to specify values for the node size if they might have to change the default settings? For example, I think that the values for s that scatter accepts are pixel values and aren't subject to max/min values. From the code, it looks like the values do not get resized to the min/max range specified, but are clipped. This might be unexpected behavior if we aren't careful.

You're right, I'm clipping the node size values with min/max. I thought that's what we were trying to do before, but we were trying to rescale them between min/max indeed. Rescaling is a better idea I agree. I wonder if we want to do it by default or not though. If you compare two hypergraphs, you can see relative difference between nodes but not absolute ones. Should we let the user deal with this, or just have it as an option? Same with min/max_node_lw.

Other issues that are potentially related to this are #174, #272, #280, and #410.

Thanks I'll look into them.

[maximelucas]

Update:

• dicts are now accepted (suggested above by Nich)
• solved node size param accepts negative values without warning #272 for drawing nodes
• added a node_shape argument
• now plotting nodes with non-finite facecolor (they were not plotted)
• fixed and added tests
• added notes in docs

Only thing left to decide:

• what to do with min/max default values for node_size etc.: clipping/interpolating/nothing, and what default? Interpolating may be nice, but I'm not sure people would expect it as default, and so may get results they don't expect. I'm thinking: do nothing by default, and maybe add an option to interpolate (but it should be explicit).

Also: the tests fail on Github saying NameError: name 'IDStat' is not defined even though IDStat is imported and tests run locally. Any idea?

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[codecov]

Codecov Report

Patch coverage: 94.73% and project coverage change: -0.03% ⚠️

Comparison is base (290729a) 91.84% compared to head (26fa109) 91.82%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #441      +/-   ##
==========================================
- Coverage   91.84%   91.82%   -0.03%     
==========================================
  Files          60       60              
  Lines        4255     4280      +25     
==========================================
+ Hits         3908     3930      +22     
- Misses        347      350       +3     

Files Changed	Coverage Δ
xgi/drawing/draw.py	74.32% <92.30%> (+0.09%)	⬆️
xgi/drawing/draw_utils.py	90.52% <100.00%> (+1.36%)	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[maximelucas]

Okay Nich I think I did everything we discussed:

• refactored with a new _draw_arg_to_arr() function
• from clipping to interpolating with a new _interp_draw_arg() function
• added a rescale_sizes argument and made the docs more explicit. For now, False by default but we can change.

I think it's ready to merge.

[nwlandry]

Looks good, Max! Thanks so much for this PR. My only small comment is that perhaps you should be consistent with the documentation on _draw_arg_to_arr. I think a more accurate descriptor would be that it makes args pyplot compliant instead of converting everything to an array?

[maximelucas]

Great, changed it. Thanks so much for reviewing this big PR Nich!