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.

Sign up for GitHub

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

Jump to bottom

More powerful setlabels #985

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

More powerful setlabels #985

wants to merge 2 commits into from

Conversation

gdementen
Copy link
Contributor

No description provided.

@gdementen gdementen requested a review from alixdamman October 11, 2021 08:48
alixdamman
alixdamman reviewed Oct 12, 2021
View reviewed changes
doc/source/changes/version_0_34.rst.inc
@@ -4,7 +4,7 @@
Syntax changes
^^^^^^^^^^^^^^

* 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()`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/renamed//

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😄

Copy link
Collaborator

Choose a reason for hiding this comment

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

Are Axis.apply() and/or Axis.replace()still mentioned in the api.rst file?
If yes, please remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

alixdamman
alixdamman reviewed Oct 12, 2021
View reviewed changes
doc/source/changes/version_0_34.rst.inc
@@ -52,6 +52,24 @@ Miscellaneous improvements
* 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
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you check if either Axis.replace() or Axis.apply() was used in the tutorial?
If yes, replace it by Axis.set_labels() please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No idea, I forgot about the tutorial.

alixdamman
alixdamman reviewed Oct 12, 2021
View reviewed changes
larray/core/axis.py
@@ -11,8 +11,9 @@

from larray.core.abstractbases import ABCAxis, ABCAxisReference, ABCArray
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,
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe mention in the commit message that you renamed tick as label?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thx

alixdamman
alixdamman reviewed Oct 12, 2021
View reviewed changes
larray/core/axis.py
# 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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will the labels of the returned axis be always of the type object?
What about non-string labels (e.g. int) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, they will. That's clearly suboptimal but already better than broken labels IMO.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So, maybe store the dtype in the beginning of the method and re-apply astype() in the end in original type was not str?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

alixdamman
alixdamman reviewed Oct 12, 2021
View reviewed changes
larray/core/group.py
@@ -343,9 +343,9 @@ def _seq_group_to_name(seq) -> Sequence[Any]:
return seq


def _to_tick(v) -> Scalar:
def _to_label(v) -> Scalar:
Copy link
Collaborator

@alixdamman alixdamman Oct 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

As said in another PR, move this function and all others of the same kind to a separate module?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unsure. Will check.

alixdamman
alixdamman approved these changes Oct 12, 2021
View reviewed changes
Copy link
Collaborator

@alixdamman alixdamman left a comment

Choose a reason for hiding this comment

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

Except my comments LGTM.

@gdementen gdementen added this to the 0.34 milestone Sep 16, 2022
@gdementen gdementen self-assigned this Sep 20, 2022
@gdementen gdementen modified the milestones: 0.34, 0.35 Sep 29, 2022
@gdementen gdementen modified the milestones: 0.35, 0.36 Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alixdamman alixdamman alixdamman approved these changes

Assignees

@gdementen gdementen

Labels
work in progress
Projects
None yet
Milestone
0.36
Development

Successfully merging this pull request may close these issues.
2 participants
@gdementen @alixdamman