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

Serialize settings for size attributes #345

Merged
merged 2 commits into from
Jan 25, 2023

Conversation

Carifio24
Copy link
Member

Serialization of size attributes currently doesn't work. We have a special column name (SIZE_COLUMN_NAME) designated for storing sizes in the serialization, but that column name is never actually used in the layer's data table. This PR updates the serialized state to match the settings messages that are dispatched here.

I'm not sure how often serialization is used, but one intended use case would be glue-viz/glue-wwt#62, as using size attributes is a common operation in glue.

@pkgw
Copy link
Contributor

pkgw commented Jan 24, 2023

Hmm. It's been a little while since I've thought about this aspect of things, so I want to be careful, but I think that this is correct. The key is that pywwt used to compute the sizes for WWT, and used the magic column name for those computed sizes. But in 9c358cf we started relying on built-in support for the engine and I think that we stopped needing to use the special column. Does that jibe with your understanding?

@Carifio24
Copy link
Member Author

Yep! That's how I had understood things as well.

@Carifio24
Copy link
Member Author

The CI failure looks like a transient timeout thing? But I suspect it would've needed another run anyways to pick up the new CI updates

@pkgw
Copy link
Contributor

pkgw commented Jan 24, 2023

I think you'll need to rebase this onto the latest master (or merge it in to your branch) for it to pick up the CI fixes.

@Carifio24
Copy link
Member Author

Just rebased

@codecov
Copy link

codecov bot commented Jan 24, 2023

Codecov Report

Merging #345 (6167f62) into master (d76265b) will increase coverage by 0.04%.
The diff coverage is 100.00%.

❗ Current head 6167f62 differs from pull request most recent head 7efa3eb. Consider uploading reports for the commit 7efa3eb to get more accurate results

@@            Coverage Diff             @@
##           master     #345      +/-   ##
==========================================
+ Coverage   56.12%   56.17%   +0.04%     
==========================================
  Files          25       25              
  Lines        2833     2836       +3     
==========================================
+ Hits         1590     1593       +3     
  Misses       1243     1243              
Impacted Files Coverage Δ
pywwt/layers.py 75.69% <100.00%> (+0.10%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@pkgw pkgw merged commit 4ae8627 into WorldWideTelescope:master Jan 25, 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.

2 participants