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] Context attributes with metas in Sieve and Mosaic #1545

Merged
merged 2 commits into from
Sep 23, 2016

Conversation

janezd
Copy link
Contributor

@janezd janezd commented Sep 6, 2016

Fixes a similar problem as #1538, but for Sieve and Mosaic.

The fix for Mosaic has no effect since context settings in Mosaic do not work. @astaric, can you check why?

We can modify Mosaic to use models (like Sieve), which would make the problem go away, but we still need to fix the bug in settings (if there is one).

@codecov-io
Copy link

codecov-io commented Sep 6, 2016

Current coverage is 88.27% (diff: 100%)

Merging #1545 into master will not change coverage

@@             master      #1545   diff @@
==========================================
  Files            77         77          
  Lines          7624       7624          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits           6730       6730          
  Misses          894        894          
  Partials          0          0          

Sunburst

Powered by Codecov. Last update ac050e1...f746440

@astaric
Copy link
Member

astaric commented Sep 8, 2016

What about setting the default value in ContextSetting to also include meta values?

@astaric
Copy link
Member

astaric commented Sep 8, 2016

What is the problem with settings in Mosaic? If I modify settings and switch between two datasets, the modified settings are restored for each dataset. What do I need to do to replicate the problem?

@janezd
Copy link
Contributor Author

janezd commented Sep 9, 2016

What about setting the default value in ContextSetting to also include meta values?

You're the context expert. If it doesn't break anything, let's change the default. Will you do it and close this PR?

This PR also fixes a smaller glitch: the default value for the selection should be set() not {}. It doesn't affect anything, though.

What is the problem with settings in Mosaic?

I can no longer replicate it either.

@astaric astaric merged commit 9f9d114 into biolab:master Sep 23, 2016
@janezd janezd deleted the sieve-context-meta branch April 5, 2019 17:31
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.

3 participants