cytoscape / RCy3

New version of RCy3, redesigned and collaboratively maintained by Cytoscape developer community
MIT License
49 stars 20 forks source link

New methods for cyjs use cases #187

Closed AlexanderPico closed 2 years ago

AlexanderPico commented 2 years ago

Re: Barry's work with Novartis use case: saving and recalling networks and styles as JSON in a database.

bdemchak commented 2 years ago

For createCytoscapejsFromNetwork, use GET /v1/networks/{networkId}/views/first so that node positions are returned. This is essential for a clean round trip.

AlexanderPico commented 2 years ago

@bdemchak Ok, I get it. I updated the original description accordingly for clarity.

I think the final discussion is about alternative names for getVisualStyle. See the last bullet under this method above. The alternative names are clearer and would alleviate some of the need to document contrasting methods, lack of symmetry with setVisualStyle, etc. Do you favor any of the suggested alts?

bdemchak commented 2 years ago

@AlexanderPico Thanks for taking the time for this ... the collaborative result is much better than I could have achieved on my own.

For getVisualStyle, the current alterego is createVisualStyle, which seems symmetric to me: 'get' vs 'create' accurately reflects what we're doing.

For the alternatives:

getVisualStyle doesn't seem to me to be deficient ... can you explain further why you'd choose a different name?

AlexanderPico commented 2 years ago

Here are my issues with getVisualStyle:

  1. The false symmetry with setVisualStyle (i.e., the common getters and setters pattern). These two functions have nothing to do with each other. The set function deals with the Visual Style as a named entity. Adding "details" to the get function would help distinguish these.
  2. The implied synonymy with getCurrentStyle. One might assume these return the same thing, but one is for the currently applied style while the other supports a styleName param. Again, adding "details" or using "extract" would help distinguish these.

These can be explained in the documentation, but compared to other methods, it seems to me that getVisualStyle would raise more than average confusion.

bdemchak commented 2 years ago

@AlexanderPico I see ... and I think I agree. Given this, would getVisualStyleAsJSON be clear enough?

AlexanderPico commented 2 years ago

Very clear! I'm also okay with getVisualStyleJSON for concision. IIRC, you may have suggested this method name weeks ago and I wasn't initially sold. Sorry :)

bdemchak commented 2 years ago

OK ... you do the honors ... I can make the change, whichever you choose. Py4cytoscape 1.4.0 is releasable at any time once we get agreement, and I get final-confirm functionality buy-in from Novartis.

AlexanderPico commented 2 years ago

Excellent. I've updated the original post in this issue to reflect the plan.

@yihangx Can you clone that plan as an issue in py4cy repo with python-syntax method names and params? And, of course, and input you have on the method and param names.

yihangx commented 2 years ago

Sure. @bdemchak I can rename these functions in py4cytoscape if you want.

bdemchak commented 2 years ago

@yihangx I have sent you the pre-release e-mail that I just sent to Novartis ... I think you'll find the names you need in there. Please feel free to ask questions or point out problems. Thanks.

yihangx commented 2 years ago

@AlexanderPico Barry implemented delete_all_visual_styles (https://github.com/cytoscape/py4cytoscape/blob/f9aa59fc756a9376747bd8f59c9a4cf92235e7d6/py4cytoscape/styles.py#L199) in py4cytoscape 1.4.0. Should we implement this method in RCy3?

AlexanderPico commented 2 years ago

yes, please

yihangx commented 2 years ago

All methods are implemented. Still need to add test cases.

yihangx commented 2 years ago

Test cases were added. I just forgot to close this issue.