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

Context handler that respects position in Domain #6740

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

markotoplak
Copy link
Member

@markotoplak markotoplak commented Feb 19, 2024

Issue

Resolves #6721

Description of changes

Added a new context handler that respects context settings' attributes exclude_attributes, exclude_metas and exclude_class_vars. This context handler avoids storing the whole domain into settings and thus perfect matches do not work.

I searched for possible instances of #6721 in this repo and found only 3 cases where a limited DomainModel was used for a context settings, out of which only 1 was problematic:

  1. owcorrelations, feature_model: not a problem, because it is applied to a processed data set where every possible feature is moved into .attributes.
  2. owtestandscore, feature_model (for CV by Feature): not the same problem, because it uses PerfectDomainHandler.
  3. owheatmap, row_side_color_model: finally, a bug. It this case, because of heavy customization, there were no crashes but UI was in an inconsistent state.

In Heat Map, a combo box for choosing row annotation color is limited to metas and class vars, and thus, when a selected feature was moved from metas to attributes, an incompatible context would match. That did not trigger any crashes because the combo box was updated through special functions, but it did leave the UI in an inconsistent state: an element with index of -1 was selected in the combo (""), and, the saved color was still shown in the map. Fixed by using the proposed ContextHandler.

Includes
  • Code changes
  • Tests
  • Documentation

@markotoplak markotoplak force-pushed the new-context-sep-classvar branch from 5286762 to ad7a9a2 Compare February 21, 2024 09:27
@markotoplak
Copy link
Member Author

I tried adding dummy .attributes and .metas to the Context, hoping that new saved contexts would also work on older versions. Testing showed this does not work: (new) saved settings can get overridden by an (old) saved perfect match.

So I can't make these compatible, at least not that simple.

A combo box for choosing row annotation color is limited to metas and
class vars, and thus, when a selected feature was moved from metas to
attributes, an incompatible context would match.

That did not trigger any crashes because the combo box was updated
through special functions, but it did leave the UI in an inconsistent state:
an element with index of -1 was selected in the combo (""), and, the saved
color was still shown in the map.
@markotoplak markotoplak force-pushed the new-context-sep-classvar branch from f626e12 to 8ae78d5 Compare July 30, 2024 11:12
Copy link

codecov bot commented Jul 30, 2024

Codecov Report

Attention: Patch coverage is 96.52174% with 4 lines in your changes missing coverage. Please review.

Project coverage is 88.27%. Comparing base (09df730) to head (f18722e).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6740   +/-   ##
=======================================
  Coverage   88.26%   88.27%           
=======================================
  Files         326      326           
  Lines       71142    71192   +50     
=======================================
+ Hits        62797    62847   +50     
  Misses       8345     8345           

@markotoplak markotoplak force-pushed the new-context-sep-classvar branch from 8ae78d5 to 6ae47bf Compare August 2, 2024 12:34
@markotoplak markotoplak force-pushed the new-context-sep-classvar branch 2 times, most recently from 6637d44 to bb6d785 Compare August 2, 2024 13:42
@markotoplak markotoplak force-pushed the new-context-sep-classvar branch from bb6d785 to 761506b Compare August 2, 2024 20:56
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.

DomainContextHandler does not handle feature "position" within the domain
1 participant