More powerful setlabels#985
Conversation
| ^^^^^^^^^^^^^^ | ||
|
|
||
| * renamed ``Array.old_method_name()`` to :py:obj:`Array.new_method_name()` (closes :issue:`1`). | ||
| * renamed ``Axis.apply()`` and ``Axis.replace()`` are deprecated in favor of :py:obj:`Axis.set_labels()`. |
There was a problem hiding this comment.
Are Axis.apply() and/or Axis.replace()still mentioned in the api.rst file?
If yes, please remove.
There was a problem hiding this comment.
You made me realize I forgot to update api.rst for this PR. So yes, they are still mentioned and Axis.set_labels is not.
| * made all I/O functions/methods/constructors to accept either a string or a pathlib.Path object | ||
| for all arguments representing a path (closes :issue:`896`). | ||
|
|
||
| * :py:obj:`Array.set_labels()` and :py:obj:`Axis.set_labels()` (formerly ``Axis.replace()`` and ``Axis.apply()``) now |
There was a problem hiding this comment.
Did you check if either Axis.replace() or Axis.apply() was used in the tutorial?
If yes, replace it by Axis.set_labels() please.
There was a problem hiding this comment.
No idea, I forgot about the tutorial.
| from larray.core.expr import ExprNode | ||
| from larray.core.group import (Group, LGroup, IGroup, IGroupMaker, _to_tick, _to_ticks, _to_key, _seq_summary, | ||
| _idx_seq_to_slice, _seq_group_to_name, _translate_group_key_hdf, remove_nested_groups) | ||
| from larray.core.group import (Group, LGroup, IGroup, IGroupMaker, _to_label, _to_labels, _to_key, _seq_summary, |
There was a problem hiding this comment.
maybe mention in the commit message that you renamed tick as label?
| # labels = self.labels.tolist() | ||
|
|
||
| # using object dtype because new labels length can be larger than the fixed str length in self.labels | ||
| labels = self.labels.astype(object) |
There was a problem hiding this comment.
Will the labels of the returned axis be always of the type object?
What about non-string labels (e.g. int) ?
There was a problem hiding this comment.
Yes, they will. That's clearly suboptimal but already better than broken labels IMO.
There was a problem hiding this comment.
So, maybe store the dtype in the beginning of the method and re-apply astype() in the end in original type was not str?
There was a problem hiding this comment.
That's not as easy, because we can easily get mixed types labels in the result (especially when making changes to only a subset of labels) even when the original dtype is not str. Imagine applying str.format() or whatever function which converts integers to strings.
There was a problem hiding this comment.
If one gets mixed types labels in the result, shouldn't we force her/him to first convert the type of the labels to str?
I'm afraid that the only case where users will get mixed types labels is when they don't realize they do.
There was a problem hiding this comment.
Converting everything to string is painful/surprising too. The usual case where you want a mixed type axis is when you have an integer axis (e.g. age) and you add a "total" label, or the opposite: you have a string axis with some special aggregate labels ("total", etc.) and want to convert all "number strings" to integers, but not the special labels.
There was a problem hiding this comment.
The usual case where you want a mixed type axis is when you have an integer axis (e.g. age) and you add a "total" label
Yes indeed. I forgot that specific case. I guess the current implementation is OK then.
|
|
||
|
|
||
| def _to_tick(v) -> Scalar: | ||
| def _to_label(v) -> Scalar: |
There was a problem hiding this comment.
As said in another PR, move this function and all others of the same kind to a separate module?
There was a problem hiding this comment.
Unsure. Will check.
alixdamman
left a comment
alixdamman
left a comment
There was a problem hiding this comment.
Except my comments LGTM.
…sions" as labels
e.g. array.set_labels({'a0:a1': 'A0..A1'}) (closes larray-project#906)
also renames all occurrences of "tick" (except for matplotlib ticks) to "label"
gdementen
force-pushed
the
906_generalize_setlabels
branch
from
305c01c to
2913978
Compare
|
More than 4 years later, I finally merged this ! 🎉 |
2 participants
No description provided.