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

style bypasses for node width and node height change current style to 'default' #114

Open
carissableker opened this issue Aug 8, 2023 · 6 comments

Comments

@carissableker
Copy link

carissableker commented Aug 8, 2023

When using the style bypasses for node width and node height,

def set_node_width_bypass(node_names, new_widths, network=None, base_url=DEFAULT_BASE_URL):

def set_node_height_bypass(node_names, new_heights, network=None, base_url=DEFAULT_BASE_URL):

the message:

 style_name not specified, so updating "default" style.

is given, and the "active" style is changed to the default one (even when I am using a custom style).

The reason seems to be the call to lock_node_dimensions, which has the argument style_name, which defaults to 'default'.

def lock_node_dimensions(new_state, style_name=None, base_url=DEFAULT_BASE_URL):

A solution could be to use get_current_style(network) before calling lock_node_dimensions, since network is already an argument to the bypasses functions.

@bdemchak
Copy link
Collaborator

bdemchak commented Aug 9, 2023

Hi, Carissa --

Thanks for catching this, and I apologize for this issue getting by us. My first thought would be to have all bypass functions accept a style parameter. Looking at the CyREST API, though, setting bypass values in a particular style isn't well supported.

Besides, your problem doesn't occur on bypass functions that don't call out to other style modules ... as you've pretty much pointed out.

So, that leads back to the solution you proposed ... these two functions do call out to other style modules, and therefore need to establish the intended style.

I'll work on this tomorrow ... the current development branch is 1.9.0, and that's where I'll put my changes. There are a few other bug fixes there ... see: https://github.com/cytoscape/py4cytoscape/blob/1.9.0/doc/release_log.rst

I'll let you know when the fix is in.

Thanks!

@yihangx @AlexanderPico

@yihangx
Copy link
Member

yihangx commented Aug 10, 2023 via email

@bdemchak
Copy link
Collaborator

@yihangx @AlexanderPico

I have checked this change into 1.9.0 so that Carissa can try it. (Carissa ... please try it)

I'm pretty sure it works, but testing it is a different story. With the pre-fix code, set_node_width_bypass calls lock_node_dimensions, which sets Cytoscape's nodeSizeLocked attribute to False, I assume this is to allow the width to be set independent of the height. (Interestingly, nodeSizeLocked never gets returned to its prior state, but that's not my problem right now ... feel free to comment on this, though.)

The upshot of Carissa's complaint would be that the nodeSizeLocked attribute was getting set on the "default" style, not the current style (e.g., "galFiltered Style"). And py4cytoscape was spitting out a warning (like RCy3 would do).

So, Carissa's fix would solve this, and that's what I checked into 1.9.0.

But the unit tests were already working fine ... with or without Carissa's fix.

I can only conclude that the natural state of a style is to have nodeSizeLocked already False. It seems that the way to test correct operation of Carissa's fix would be to have nodeSizeLocked set to True on the current style so that node_lock_dimensions(False) would actually do something to avoid an error.

Can you tell me whether the above argument holds water ... and how to tell if nodeSizeLocked is set to cause trouble?

Pretty subtle, but I hate releasing something that's not clearly testable.

@bdemchak
Copy link
Collaborator

... or is the nodeSizeLocked lockout enforced only in the Cytoscape GUI and not at the CyREST level?

@carissableker
Copy link
Author

Hi Barry,

Thanks for the fast fix and detailed discussion!
In my case, I did not change the nodeSizeLocked setting, and in the GUI the "Lock node width and height" checkbox (I'm assuming the equivalent of nodeSizeLocked) was unchecked (but now I realise I am not sure to which style this refers, maybe in the default style it was checked at the time...).

Your fix works perfectly in my case. I also tested it with "Lock node width and height" checked, and it unchecked it quietly.

In the GUI, it seems the width and height bypasses and size lock play together rather confusingly. With "Lock node width and height" checked the width and height attributes are inaccesible.

But:

  • Uncheck "Lock node width and height"
  • Make an bypass in width and height for an individual node
  • Check "Lock node width and height"
  • The bypass is still applied

Maybe in the py4cytoscape bypass implementation, it would make sense, once the bypass is applied, to revert the nodeSizeLocked state to what it was before? I would find that more obvious, since I would not expect applying a bypass to a single node to change the style settings. I think bypasses are, at least meant to be, independent of the style. Indeed, changing the style preserves the bypass.

I think this is perhaps a more fundamental Cytoscape issue -- the node size lock should not affect the ability to apply width and height bypasses, as it is circumventable anyway.

Thanks again! For my use case, this is solved.

@bdemchak
Copy link
Collaborator

Excellent, Carissa ... thanks for your help!

I agree with your intuition regarding nodeSizeLocked, and my only issue with that is to explain why the existing acceptance tests were passing in the first place. These questions are for the Cytoscape crew to explore.

I'd give good odds that there could be further tweaks, so I'll leave this issue open so you see its progress.

Thanks for your patience and help!

@bdemchak bdemchak reopened this Sep 27, 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

No branches or pull requests

3 participants