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

Fix IDView.ids type #406

Merged
merged 2 commits into from
Jun 21, 2023
Merged

Fix IDView.ids type #406

merged 2 commits into from
Jun 21, 2023

Conversation

leotrs
Copy link
Collaborator

@leotrs leotrs commented Jun 19, 2023

Currently, H.edges.ids sometimes returns a dict and sometimes returns a set:

>>> import xgi
>>> H  = xgi.Hypergraph([{1, 2, 3}, {4}, {5, 6}, {6, 7, 8}])
>>> H.edges.ids
{0: {1, 2, 3}, 1: {4}, 2: {5, 6}, 3: {6, 7, 8}}
>>> H.edges({0, 1, 2}).ids
{0, 1, 2}

This is terrible. Furthermore, the dict version can be obtained via members:

>>> H.edges.members(dtype=dict)
{0: {1, 2, 3}, 1: {4}, 2: {5, 6}, 3: {6, 7, 8}}

This PR makes it so that it always returns a set.

>>> import xgi
>>> H  = xgi.Hypergraph([{1, 2, 3}, {4}, {5, 6}, {6, 7, 8}])
>>> H.edges.ids
{0, 1, 2, 3}

I also added a couple of tests and changed an old test that relied on ids returning a dict (yuck).

…ealth with in __init__. Also add tests and use check_like=True in an old test which ignores row order.
@codecov
Copy link

codecov bot commented Jun 19, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.02 🎉

Comparison is base (6f04d2f) 90.95% compared to head (24b6c73) 90.97%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #406      +/-   ##
==========================================
+ Coverage   90.95%   90.97%   +0.02%     
==========================================
  Files          47       47              
  Lines        3893     3891       -2     
==========================================
- Hits         3541     3540       -1     
+ Misses        352      351       -1     
Impacted Files Coverage Δ
xgi/classes/reportviews.py 94.05% <100.00%> (+0.47%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@nwlandry
Copy link
Collaborator

Nice work! Can you comment on Issue #362? For now maybe you could remove the comment in the notebook about order being preserved?

@leotrs
Copy link
Collaborator Author

leotrs commented Jun 19, 2023

@nwlandry done!

@nwlandry
Copy link
Collaborator

Looks good to me! :)

@nwlandry
Copy link
Collaborator

nwlandry commented Jun 20, 2023

Okay, one more thing. I noticed that in the constructor of IDView, self._ids doesn't have to be a set (line 91). Can you fix that before merging?

@leotrs
Copy link
Collaborator Author

leotrs commented Jun 20, 2023

Could you be more specific? L91 and subsequent lines guarantee that self._ids is always set, regardless of the value of ids.

@nwlandry
Copy link
Collaborator

Could you be more specific? L91 and subsequent lines guarantee that self._ids is always set, regardless of the value of ids.

In this PR, I don't believe that self._ids is always a set if you construct an IDView with a) ids=None or b) (for example) ids=[1, 2, 3]. I would do something like

self._ids = set(self._id_dict) if ids is None else self._ids = set(ids)

on L91, to enforce this.

@leotrs
Copy link
Collaborator Author

leotrs commented Jun 20, 2023

Perhaps we're not seeing the same code? On this PR, L91 and subsequent lines look like this:

        if ids is None:
            self._ids = self._id_dict
        else:
            self._ids = ids

@nwlandry
Copy link
Collaborator

I tried out a few things and I was mistaken. I take back my prior comment :)

@leotrs leotrs merged commit b5d41dd into main Jun 21, 2023
19 checks passed
@leotrs leotrs deleted the fix-ids branch June 21, 2023 07:51
@leotrs leotrs mentioned this pull request Jun 21, 2023
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.

None yet

2 participants