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

Allow specifying visual attributes when creating subset group #2297

Merged
merged 6 commits into from
May 19, 2022

Conversation

Carifio24
Copy link
Member

This PR allows specifying visual attributes when creating a new subset group, either via the SubsetGroup initializer or the DataCollection::new_subset_group method.

I don't know that this adds much, if any, utility to the standard glue workflow - the intended use case here is for dashboard-style applications like Cosmic Data Stories. In a setting where the application is doing the styling, the delay between creating the new subset group and modifying the styling is occasionally visible, which this PR is looking to solve.

@codecov
Copy link

codecov bot commented May 12, 2022

Codecov Report

Merging #2297 (4b7c005) into main (8bc18e3) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2297   +/-   ##
=======================================
  Coverage   88.15%   88.15%           
=======================================
  Files         247      247           
  Lines       23325    23327    +2     
=======================================
+ Hits        20563    20565    +2     
  Misses       2762     2762           
Impacted Files Coverage Δ
glue/core/data_collection.py 90.33% <100.00%> (ø)
glue/core/subset_group.py 99.15% <100.00%> (+<0.01%) ⬆️
glue/core/visual.py 87.38% <100.00%> (+0.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8bc18e3...4b7c005. Read the comment docs.

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! Could you add a test though?

@Carifio24 Carifio24 force-pushed the subset-group-visual branch from 32c8909 to 5471066 Compare May 19, 2022 05:04
@Carifio24
Copy link
Member Author

@astrofrog I've added two tests - one to check that the correct default values are applied in new_subset_group, and another with an explicit set of visual attributes. Let me know if there are any other sorts of tests that you think would be good to add.

I've also changed the default value for markersize in the SubsetGroup constructor to 7, rather than 7.5, since the VisualAttributes setter converts it into a int anyways. (I had originally put in 7.5 to match the existing value that was assigned - the default value for markersize in VisualAttributes is 3, and the current code for new_subset_group multiplies the markersize by 2.5).

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, thanks!

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.

2 participants