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

Add regiondata #2442

Merged
merged 20 commits into from
Oct 23, 2023
Merged

Add regiondata #2442

merged 20 commits into from
Oct 23, 2023

Conversation

jfoster17
Copy link
Member

Add RegionData and ExtendedComponent Types

Description

This is a replacement for #2373 which provides just the glue-core pieces of that PR and some additional improvements. As a standalone piece it doesn't do very much, but it provides the underlying architecture so that Viewers can be extended to deal with regions-as-data.

The pieces here are:

  • An ExtendedComponent(Component) which takes a list of Shapely geometries and a list of ComponentIDs describing the center of these geometries.
  • A RegionData(Data) object which has one (and only one) ExtendedComponent and is the general class to use for any regions-as-data.

Some additions over #2373 are that this no longer requires us to allow_pickle in np.load and it properly round-trips through session files, albeit with a bit of a hack to get the ComponentIDs to be all correct.

Closes #2351 , #2086 , #1686

@jfoster17 jfoster17 requested a review from astrofrog September 13, 2023 20:56
@jfoster17 jfoster17 mentioned this pull request Sep 13, 2023
3 tasks

result._extended_component_id = context.object(rec["_extended_component_id"])

def fix_special_component_ids(ext_data):
Copy link
Member Author

Choose a reason for hiding this comment

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

@astrofrog -- this is a hacky function I would like to eliminate, but I'm not sure how to rewrite the save/load logic to deal with the underlying problem. The basic problem is that when I restore a session, the ComponentIDs stored inside of an ExtendedComponent are no longer the same as the ComponentIDs for the actual components they reference. I think they are being created separately. This hacky function works, but would fail if there are multiple attributes with the same name/label.

Copy link
Member

Choose a reason for hiding this comment

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

Can you save the UUIDs of these attributes when saving the session, then you can more easily match it up to the correct attribute not relying on the name when you re-load them?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that should work. Thanks for the idea!

Copy link
Member Author

Choose a reason for hiding this comment

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

This solution involves saving ComponentID uuids into the session file and restoring them (if present) from the session file. I don't think this introduces and backwards-incompatibility problems (all tests pass) but does that seem okay to you?

Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

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

Looks great so far! Just a few comments.

glue/core/component.py Outdated Show resolved Hide resolved
glue/core/data.py Outdated Show resolved Hide resolved
glue/core/data.py Outdated Show resolved Hide resolved
glue/core/state.py Outdated Show resolved Hide resolved

result._extended_component_id = context.object(rec["_extended_component_id"])

def fix_special_component_ids(ext_data):
Copy link
Member

Choose a reason for hiding this comment

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

Can you save the UUIDs of these attributes when saving the session, then you can more easily match it up to the correct attribute not relying on the name when you re-load them?

@jfoster17
Copy link
Member Author

@astrofrog -- I think this is ready now. I would love to get it merged in before opening the PRs for adding RegionData to the viewers.

Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

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

Looks good now, thanks!

@astrofrog astrofrog merged commit 4f7efcb into glue-viz:main Oct 23, 2023
21 checks passed
@jfoster17 jfoster17 deleted the add-regiondata branch October 31, 2023 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add an ExtendedComponent to glue
2 participants