Allow visualization of shapes in the structure viewer

[rosecers]

In response to #275

Soliciting feedback.

Right now I have envisioned it as we have a set dictionary of supported shapes, right now spheres (redundant, yes, but they are useful for testing) and ellipsoids, located in src/shapes.ts. These classes each should have their own static validation method that can be called in src/dataset.ts and a method that generates the input for 3dmol.addCustom. During rendering in src/structure/viewer.ts, the viewer loads these methods and pushes a new instance to the viewer. If shapes are being used, we do not render the corresponding sphere, but this can be switched on/off.

Changes have been made to the following files:

• src/dataset.ts: adding shape parameters to the Structure, validating the shape parameters when validating the structure
• src/3dmol/index.d.ts: adding CustomShapeSpec and addCustom for 3dmol
• src/structure/options*: button to turn shape on/off
• src/structure/viewer.ts: function to add shapes to the viewer, disabling sphere style when shapes are wanted

As of now it is not working. You'll see in the ellipsoid class in src/shapes.ts I've put in some dummy coordinates to see if 3dmol will render a simple cube, but to no avail. Would appreciate feedback and input on 1) general plan of attack and 2) why it is not rendering

[rosecers]

A couple of code comments. Regarding why it is not rendering, the code seems to be missing an update of this.shape_ whenever the user updates the checkbox, did you try forcing shapes to be true? (I would remove this checkbox altogether anyway).

If that does not help, I would check that 3Dmol shape rendering works in a standalone example, using https://codepen.io/luthaf/pen/JjmvKVe as a stating point for example.

I did try forcing this._shape=true in the constructor (while also overriding the options), but to no avail. I tried the codepen, but was also having issues getting it to compile (not confident on that interface).

I'd like to keep the checkbox, it can sometimes be important to render the shapes or their centers, depending on what you're looking for.

Any other thoughts on why it isn't rendering?

[Luthaf]

Any other thoughts on why it isn't rendering?

Try removing this._shape and using this.options.shape.value instead, this should removing the need to synchronize with HTML manually

[rosecers]

I've given another pass and... it works! There's still some small things to work out in the code, but @Luthaf can you take a look at the general changes and see how you feel? I had to rebase on #283 , because it turns out there were some issues with us templating the addCustomShape into the 3dmol file from earlier.

[rosecers]

Hey @Luthaf, the code above caused a bunch of crashes, so I had to revert a bunch. What of this is suggestions and what is merge-necessary requirements? I ensure anything that is purely for example will be removed pre-merge.

[ceriottm]

Something that would make this (even) more useful is to give the possibility of specifying multiple shapes, and then choosing which shape to show. This would allow one to use this to visualize properties - polarizability, forces, etc. A "scale" slider would then also be very useful but it can come later. Thoughts?

[rosecers]

More updates! Now we can support multiple shapes for a given dataset, with a drop-down to select them. Was a pain to implement, but I think it came out quite nicely. Ready for another look-over!

[rosecers]

Hey @Luthaf , I handled most change requests, leaving one or two questions along the way. The remaining API iron-out is whether shapes should be passed through the frames or no -- how much of a hang-up is this? Full stop or open for discussion?

[Luthaf]

The remaining API iron-out is whether shapes should be passed through the frames or no -- how much of a hang-up is this? Full stop or open for discussion?

I would like to get this right before merging, but I'm willing to help implement it when we converge on the API design!

Other than that, this PR also needs some documentation in https://github.com/lab-cosmo/chemiscope/blob/master/docs/src/tutorial/input-reference.rst, with a short example for each shape kind.

We also need to reach some consensus on the quaternion convention, but then the code looks ready to go!

[Luthaf]

Note for myself: check what happen when the users switches between different structures with different set of shapes (i.e. different shape names).

[rosecers]

@Luthaf I've made the switch in the python API, let me know thoughts. I added docs and clarity on quaternions. Should be ready for next review.

[rosecers]

I tried the code locally, and it runs quite nicely!

A problem occurs when trying to load a file with different shape names in each structure, and switching to a structure where the previous shape is not defined (here a small file demonstrating this: multiple-shapes.txt)

!(current_shape === undefined)
_construct@http://localhost:8080/chemiscope-app.min.js:53236:393
Wrapper@http://localhost:8080/chemiscope-app.min.js:53232:473
AssertionError@http://localhost:8080/chemiscope-app.min.js:53577:80
innerOk@http://localhost:8080/chemiscope-app.min.js:52712:15
ok@http://localhost:8080/chemiscope-app.min.js:52731:11
_addShapes@http://localhost:8080/chemiscope-app.min.js:219093:54
_updateStyle@http://localhost:8080/chemiscope-app.min.js:219063:18
load@http://localhost:8080/chemiscope-app.min.js:218713:14
_showInViewer@http://localhost:8080/chemiscope-app.min.js:217934:20
show@http://localhost:8080/chemiscope-app.min.js:217717:14
./src/index.ts/DefaultVisualizer/this.map.onselect@http://localhost:8080/chemiscope-app.min.js:212338:28
./src/map/map.ts/_createPlot/<@http://localhost:8080/chemiscope-app.min.js:215294:18
emit@http://localhost:8080/chemiscope-app.min.js:69404:17
./node_modules/plotly.js/src/lib/events.js/init/plotObj.emit@http://localhost:8080/chemiscope-app.min.js:114817:16
emitClick@http://localhost:8080/chemiscope-app.min.js:93491:31
click@http://localhost:8080/chemiscope-app.min.js:93496:16
clickFn@http://localhost:8080/chemiscope-app.min.js:136496:20
onDone@http://localhost:8080/chemiscope-app.min.js:89897:41
mouseupListener@http://localhost:8080/chemiscope-app.min.js:216324:26

We could decide this is an invalid file, and check that all structures have the same set of shape, or we could handle the case when loading a new structure. I would tend toward the first solution, and we can always lift the restriction later if needed.

I would say to make this invalid to avoid the nan disaster of partially-defined features, what do you think?

[ceriottm]

Just to say I tried it and (with errors) it works. Just a few superficial comments: 1) it would be nice to have an example that demonstrates ALL functionalities, not just the ellipsoids 2) I strongly suggest that we should allow to display the shapes in addition to the atoms. must think of a good UI for that 3) IDK if it's intended, but all structures are shown with ellipsoids, even when there are none defined (e.g. for the default examples) 4) Is there an example of how to use this from a jupyter notebook?

[Luthaf]

I would say to make this invalid to avoid the nan disaster of partially-defined features, what do you think?

yes, making it invalid sounds good!

[Luthaf]

I just squashed all commits from this PR in a single one, and removed the example file. Here it is for posterity: sample_ellipsoids.json.txt

[Luthaf]

I've done a bunch of cleanup, a couple of remaining questions/discussion points:

• giving non-normalized quaternions for the orientations renders weirdly. Should we warn/error on them?

chemiscope.write_input(
    "1.json",
    frames=[frame],
    properties={
        "foo": [1.2],
        "bar": [1.5],
    },
    shapes={
        "cubes": [
            [
                {"kind": "custom", "vertices": CUBE_VERTICES},
                {
                    "kind": "custom",
                    "vertices": CUBE_VERTICES,
                    "orientation": [1, 0, 0, 0],
                },
                {
                    "kind": "custom",
                    "vertices": CUBE_VERTICES,
                    "simplices": CUBE_SIMPLICES,
                    "orientation": [0.1, 0.2, 0, 0],
                },
            ],
        ]
    },
)

• the file above is still a bit weird w.r.t. shading and light. Is this a convention issue for the order of simplices?

Still TBD for me: adding more validation of the input JSON on the JS side.

[rosecers]

I've done a bunch of cleanup, a couple of remaining questions/discussion points:

• giving non-normalized quaternions for the orientations renders weirdly. Should we warn/error on them?

chemiscope.write_input(
    "1.json",
    frames=[frame],
    properties={
        "foo": [1.2],
        "bar": [1.5],
    },
    shapes={
        "cubes": [
            [
                {"kind": "custom", "vertices": CUBE_VERTICES},
                {
                    "kind": "custom",
                    "vertices": CUBE_VERTICES,
                    "orientation": [1, 0, 0, 0],
                },
                {
                    "kind": "custom",
                    "vertices": CUBE_VERTICES,
                    "simplices": CUBE_SIMPLICES,
                    "orientation": [0.1, 0.2, 0, 0],
                },
            ],
        ]
    },
)

• the file above is still a bit weird w.r.t. shading and light. Is this a convention issue for the order of simplices?

Still TBD for me: adding more validation of the input JSON on the JS side.

I added a test for quaternion normalization -- I honestly have no idea why the cubes look weird.

[Luthaf]

The code is now in good shape IMO. Are you also happy with it @rosecers? If so, we could merge this, wait for a couple of weeks to let any remaining issues surface (and try to figure out what's happening with the cubes) and then release version 0.6 of chemiscope!

EDIT: I'll fix the remaining issue in the Python code

[rosecers]

The code is now in good shape IMO. Are you also happy with it @rosecers? If so, we could merge this, wait for a couple of weeks to let any remaining issues surface (and try to figure out what's happening with the cubes) and then release version 0.6 of chemiscope!

EDIT: I'll fix the remaining issue in the Python code

Sounds good!

[ceriottm]

Do you think we could also complete the color-by-property PR? I think the two would go together VERY well.

On Fri, 21 Jul 2023 at 09:58, Rose K. Cersonsky @.***> wrote:

The code is now in good shape IMO. Are you also happy with it @rosecers https://github.com/rosecers? If so, we could merge this, wait for a couple of weeks to let any remaining issues surface (and try to figure out what's happening with the cubes) and then release version 0.6 of chemiscope!

EDIT: I'll fix the remaining issue in the Python code

Sounds good!

— Reply to this email directly, view it on GitHub https://github.com/lab-cosmo/chemiscope/pull/282#issuecomment-1645801351, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIREZY7OOLKC6HO5Y5YPQTXRKRLRANCNFSM6AAAAAAX5DTVBA . You are receiving this because you commented.Message ID: @.***>

[Luthaf]

Since the idea was that the new master student would work on the color-by-property branch, that would push the release quite a bit further. I'd be fine to do a v0.6.1 or v0.7 with color-by-property while making the shapes available to everyone sooner.

[ceriottm]

OK makes sense.

On Mon, 24 Jul 2023 at 02:53, Guillaume Fraux @.***> wrote:

Since the idea was that the new master student would work on the color-by-property branch, that would push the release quite a bit further. I'd be fine to do a v0.6.1 or v0.7 with color-by-property while making the shapes available to everyone sooner.

— Reply to this email directly, view it on GitHub https://github.com/lab-cosmo/chemiscope/pull/282#issuecomment-1647491881, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAIREZ7Z7MPQN66NDQCHPIDXRYZXRANCNFSM6AAAAAAX5DTVBA . You are receiving this because you commented.Message ID: @.***>