Fix the bugs by using Dict for the selected_elements #14

dou-du · 2020-05-23T15:53:13Z

In order to fix the bugs mentioned in issue #4 , I decide to use a Dict (traitlets) for the selected_elements.

widget.selected_elements = {"H":0, "O":1}

However, there is a issue for the Dict.

#This will not work
widget.selected_elements["H"] = 2

…into develop

dou-du · 2020-05-23T15:57:57Z

@giovannipizzi
Please make a review for this pull request.

Sphinx.warn() was deprecated since 1.6 and removed at 2.0.

…into develop

CasperWA

Great with some updates on this widget! :)

Although I still have some issues using it to its fullest potential.

CasperWA · 2020-07-08T18:53:22Z

src/widget.ts

@@ -81,25 +81,24 @@ class MCPTableView extends DOMWidgetView {
  // http://codebeerstartups.com/2012/12/how-to-improve-templates-in-backbone-js-learning-backbone-js/
  tableTemplate =  _.template( '<% for (let elementRow of elementTable)'+
  ' { print("<div class=\'periodic-table-row\'>"); for (let elementName of elementRow)'+
-  ' { if ( (elementName === "") || (elementName == "*" ) ) { %>' +
+  ' { if ( (elementName === "") || (elementName == "*" ) || (elementName == "#" ) ) { %>' +


I guess * is not needed anymore due to the change above from * -> #?

Suggested change

' { if ( (elementName === "") || (elementName == "*" ) || (elementName == "#" ) ) { %>' +

' { if ( (elementName == "") || (elementName == "#" ) ) { %>' +

I used both "#" and "*" to separate lanthanides and actinides group.

CasperWA · 2020-07-08T18:53:59Z

src/widget.ts

+  // 'title="state: <% if (selectedElements.includes(elementName)) { i = selectedElements.indexOf(elementName); print(selectedStates[i]);} '+
+  // 'else if (disabledElements.includes(elementName)){print("disabled");} else {print("unselected");} %>" ><% '+


Why the commenting out?

Giovanni suggested to remove the tooltips from the periodic table.

src/widget.ts

CasperWA · 2020-07-08T18:55:48Z

src/widget.ts

+      // I have to make some changes, since there is some issue
+      // for Dict in Traitlets, which cannot triggle the update


I don't believe this is true. I think you just need to set a default value of {} for the traitlet or something, right?

I think the problem is that if it was already {}, no message is sent back.

No only for empty dict. For example newStateList was {"H":1, "O": 2} then

newSateList = {"H": 3, "O": 1}

will not trigger the update in the python end PTable.selected_elements will show {"H":1, "O": 2}

CasperWA · 2020-07-08T18:56:24Z

src/widget.ts

-      this.model.set('selected_states', newStatesList);
+      // I have to make some changes, since there is some issue
+      // for Dict in Traitlets, which cannot triggle the update
+      this.model.set('selected_elements', {"Du": 0});


Why this? Shouldn't it be set to the actually selected elements?

This is a dirty trick to force the update for the PTable.selected_elements as mentioned above.

CasperWA · 2020-07-08T18:56:57Z

src/widget.ts

+      // while (newSelectedElements.length > newSelectedStates.length){
+      //   newSelectedStates.push(0);
+      // };


Why the commenting out?

This is for previous code, when using list for the selected_elements and selected_states. For the dictionary, it does not need anymore.

widget_periodictable/periodic_table.py

CasperWA · 2020-07-08T19:01:28Z

widget_periodictable/periodic_table.py

+            self.selected_colors = self.selected_colors[:states]
+
+
+    def set_element_state(self, elementName, state):


I don't think this function is ever used. Hence the functionality of the widget is non-existing at the moment.

This function is import to set the element states.

For example:

PTable.selected_elements["H"] = 2

will not work.

On can use this function to set individual element's state.

CasperWA · 2020-07-09T11:59:35Z

@giovannipizzi
Please make a review for this pull request.

@dou-du you can assign people for review on the right hand side of this page (the "Reviewers" section).

giovannipizzi

Minor comments.
I couldn't check the code in detail, though. Can I check it on binder already, I guess?

giovannipizzi · 2020-07-09T13:04:13Z

examples/introduction.ipynb

  {
   "cell_type": "code",
-   "execution_count": null,
+   "execution_count": 10,


Are you committing on purpose a run notebook?

I just commit to save all change. This is not needed to be committed. I will change it back.

giovannipizzi · 2020-07-09T13:04:42Z

postBuild

@@ -1,6 +1,6 @@
 #!/bin/bash
-jupyter labextension install @jupyter-widgets/jupyterlab-manager 
+pip install -i https://test.pypi.org/simple/ widget-periodictable


Do you want to suggest to install from test-pypi?

I just use it to test the widget-periodictable package. I have released a new version at PyPi. I will change it to PyPi.

giovannipizzi · 2020-07-09T13:51:30Z

src/widget.ts

+      // I have to make some changes, since there is some issue
+      // for Dict in Traitlets, which cannot triggle the update


I think the problem is that if it was already {}, no message is sent back.

giovannipizzi · 2020-07-09T14:11:38Z

I checked on Binder, seems to be working fine!

I think it would be also useful to have a function widget.unselect_element(elementName), but it's not critical for now, there are various ways to achieve so, e.g. disable and reenable the element (even if it's inelegant) or things like

tmp = widget.selected_elements.copy()
tmp.pop(elementName)
widget.selected_elements = tmp

Co-authored-by: Casper Welzel Andersen <[email protected]>

dou-du · 2020-07-09T16:38:21Z

Thanks @CasperWA and @giovannipizzi

I will have a final checking about the code and approve the pull request.

dou-du added 12 commits May 8, 2020 01:41

Update the example file

557a453

Merge branch 'develop' of github.com:osscar-org/widget-periodictable …

7af1853

…into develop

Add Logo to the page

abbf979

Fix the bugs for issue #4

ce79826

JupyterLab 2.1.1

0d1dfaa

Fix the bug by adding the extension.js (important)

c05d00c

More changes for issue #4

31ebe91

Commit for mybinder

ba11a7b

New update by using a dictionary

102a35e

Use dictionary to fix issue #4

d7bf982

Fix one more bug for issue #4

d1e193c

Update the introduction.ipynb

1a568f2

dou-du and others added 5 commits May 26, 2020 17:33

Update conf.py

5e81293

Sphinx.warn() was deprecated since 1.6 and removed at 2.0.

Update the examples for docs

9b9fdd1

Merge branch 'develop' of github.com:osscar-org/widget-periodictable …

9f443e0

…into develop

Fix the bug for Disable elements conflication with selected_elements

4fb237c

Relase a new version 2.0.0

72602d2

CasperWA reviewed Jul 8, 2020

View reviewed changes

giovannipizzi reviewed Jul 9, 2020

View reviewed changes

dou-du and others added 2 commits July 9, 2020 18:10

Update src/widget.ts

8d42896

Co-authored-by: Casper Welzel Andersen <[email protected]>

Update widget_periodictable/periodic_table.py

e439c5d

Co-authored-by: Casper Welzel Andersen <[email protected]>

Update the introduction.ipynb

8a91b62

dou-du merged commit 2e623fd into master Jul 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the bugs by using Dict for the selected_elements #14

Fix the bugs by using Dict for the selected_elements #14

dou-du commented May 23, 2020

dou-du commented May 23, 2020

CasperWA left a comment

CasperWA Jul 8, 2020

dou-du Jul 9, 2020

CasperWA Jul 8, 2020

dou-du Jul 9, 2020

CasperWA Jul 8, 2020

giovannipizzi Jul 9, 2020

dou-du Jul 9, 2020

CasperWA Jul 8, 2020

dou-du Jul 9, 2020

CasperWA Jul 8, 2020

dou-du Jul 9, 2020

CasperWA Jul 8, 2020

dou-du Jul 9, 2020

CasperWA commented Jul 9, 2020 •

edited

Loading

giovannipizzi left a comment

giovannipizzi Jul 9, 2020

dou-du Jul 9, 2020

giovannipizzi Jul 9, 2020

dou-du Jul 9, 2020

giovannipizzi Jul 9, 2020

giovannipizzi commented Jul 9, 2020

dou-du commented Jul 9, 2020

	' { if ( (elementName === "") \|\| (elementName == "*" ) \|\| (elementName == "#" ) ) { %>' +
	' { if ( (elementName == "") \|\| (elementName == "#" ) ) { %>' +

		// 'title="state: <% if (selectedElements.includes(elementName)) { i = selectedElements.indexOf(elementName); print(selectedStates[i]);} '+
		// 'else if (disabledElements.includes(elementName)){print("disabled");} else {print("unselected");} %>" ><% '+

		// I have to make some changes, since there is some issue
		// for Dict in Traitlets, which cannot triggle the update

		self.selected_colors = self.selected_colors[:states]


		def set_element_state(self, elementName, state):

Fix the bugs by using Dict for the selected_elements #14

Fix the bugs by using Dict for the selected_elements #14

Conversation

dou-du commented May 23, 2020

dou-du commented May 23, 2020

CasperWA left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CasperWA commented Jul 9, 2020 • edited Loading

giovannipizzi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giovannipizzi commented Jul 9, 2020

dou-du commented Jul 9, 2020

CasperWA commented Jul 9, 2020 •

edited

Loading