Support TrajectoryData in StructureManagerWidget

[danielhollas]

Use case

In the app that I am building, the workchain does not start for a single structure, but from a set of molecular conformers, which I am representing with TrajectoryData. I have build a TrajectoryDataViewer, which inherits from StructureDataViewer. In this PR I am patching up the StructureManagerWidget so that I can use the new viewer.

Implementation

The current code is a bit hacky and relies on the fact that it does not support structure editors. I consider the full support outside of the scope for this PR, hopefully it is useful on it's own and is not too invasive to the common use case. I raise the exception if user tries to use editors.

Let me know if there is interest in upstreaming the TrajectoryDataViewer. The current implementation is fairly barebones and is not optimized for speed, e.g. for looking at MD trajectories, since the target use case is to have a handful of conformers. You can take a look here:

https://github.com/danielhollas/aiidalab-ispg/blob/0436cc814f104438b3d8818ca47d66c75e01d3e5/aiidalab_ispg/widgets.py#L221

I took inspiration from #281, thanks @yakutovicha

EDIT: I have also patched the UploadWidget to support uploading multiple structures into a single file if node_class == TrajectoryData, but perhaps this merits a separate PR as there seem to be some subtleties. Also the current code in UploadWidget does not handle errors well.

[csadorf]

@yakutovicha Do you have capacity to review?

[yakutovicha]

@yakutovicha Do you have capacity to review?

Yes, I am going to have a look on Monday.

[yakutovicha]

Thanks, @danielhollas!

After going through the PR I would like to discuss the potential use case you are trying to address. Why would you like to enable the StructureManagerWidget to handle the trajectory? Would you like to modify it somehow?

I fear that the structure manager widget gets overloaded with adding support to the trajectory data. Dealing with one structure is easy and clear while dealing with trajectory can get pretty messy.

If your use case is to be able to scroll through the frames, select one, modify it (if needed), and use it as the input of the other simulation we might do something easier. For example, one could add a scrolling feature to the UploadStructure widget. That would already cover the usecase I mentioned earlier. Similar could be done for the StructureUploadWidget.

So to summarise, unless there is a clear reason to do so, I wouldn't give to StructureManagerWidget the ability to natively handle TrajectoryData. We had this before and we went away from it (too complex setup with very little outcome).

Let me know your thoughts on this.

[danielhollas]

Hi @yakutovicha, thanks for looking into this. I am afraid my use case is slightly different, I actually am passing the TrajectoryData into the next workflow step. In other words, my workflow takes TrajectoryData as input, since my workflow operates on a set of molecular conformers (e.g. same molecule, but different XYZ coordinates).

As stated in the OP, at least for now I don't need the ability to modify the TrajectoryData, but I need to be able to create it, either from Upload Widget or SmilesWidget, select it from AiiDA DB.

I guess if this PR doesn't land, I could subclass the widget?

class TrajectoryManagerWidget(StructureManagerWidget):

But then I guess I would also need to have my own versions of the importer widgets.

[danielhollas]

@yakutovicha I would have a suggestion. Perhaps I could split this PR and submit changes that would be imo nice to have regardless of direct TrajectoryData support, and we could go from there? Essentially I would be happy if at least part of this got merged so that I don't have to maintain every widget in my repo.

[yakutovicha]

As stated in the OP, at least for now I don't need the ability to modify the TrajectoryData, but I need to be able to create it, either from Upload Widget or SmilesWidget, select it from AiiDA DB.

Sorry, I jumped right to the code and forgot to read the description, you've provided quite a number of answers there.

@yakutovicha I would have a suggestion. Perhaps I could split this PR and submit changes that would be imo nice to have regardless of direct TrajectoryData support, and we could go from there? Essentially I would be happy if at least part of this got merged so that I don't have to maintain every widget in my repo.

Yep, I like this idea. Starting simple always gives us room for improvement and more time to think about better organisation.

Now, after I properly read the description I would propose to do the following:

• Add to UploadStructure the ability to upload the trajectory from a file. If the tool detects multiple frames, it enables (or better, displays) the "Save as Trajectory" button, plus info message and the frame selector. Adding the ability to select a specific frame would let us display it in the viewer. Something like this, very schematically:

You've essentially done it already in this PR. A couple of additional widgets would complete this part of the work.

• Maybe add a similar thing to the StructureBrowseWidget: show trajectory data in the dropdown, allow to select a frame.

• A trajectory viewer is also a great thing to have. I didn't manage to complete Working version of trajectory visualizer based on bqplot #281, and I would be very grateful if you could do it.

I believe this covers all the cases we have discussed.

I guess if this PR doesn't land, I could subclass the widget?

Let's keep this in mind. It feels like overkill, but might be the way to go, in the end.

[danielhollas]

Okay, sound good, I'll work on this in pieces.

Add to UploadStructure the ability to upload the trajectory from a file. If the tool detects multiple frames, it enables (or better, displays) the "Save as Trajectory" button, plus info message and the frame selector. Adding the ability to select a specific frame would let us display it in the viewer. Something like this, very schematically:

This sounds great, much better than mu current approach. I need one clarification however: a button for "Save all as Trajectory" already supposes that StructureManagerWidget supports it? And I guess we should only display it only if node_class == TrajectoryData, e.g. only when it makes sense to pass this into a workflow?

In this PR, my assumption is that when the StructureManagerWidget is initiated with node_class='TrajectoryData, then it will actually accept both StructureData and TrajectoryData as valid inputs, but the converse is not true.

[yakutovicha]

This sounds great, much better than mu current approach. I need one clarification however: a button for "Save all as Trajectory" already supposes that StructureManagerWidget supports it? And I guess we should only display it only if node_class == TrajectoryData, e.g. only when it makes sense to pass this into a workflow?

In this PR, my assumption is that when the StructureManagerWidget is initiated with node_class='TrajectoryData, then it will actually accept both StructureData and TrajectoryData as valid inputs, but the converse is not true.

Hmm, let me think about it a little bit more. I kind of ignored the point that you want to pass TrajectoryData as an input to your workflow. That makes things a little bit different. Maybe having a separate class wouldn't be a bad idea, in the end.

[danielhollas]

@yakutovicha let me know in case you want to hop on a quick call (in that case I could actually show you how it works). In the meantime I will cook up individual PRs from this (probably tomorrow).

[yakutovicha]

@yakutovicha let me know in case you want to hop on a quick call (in that case I could actually show you how it works). In the meantime I will cook up individual PRs from this (probably tomorrow).

yes, it is a good idea. I will contact you via email.

[danielhollas]

@yakutovicha thanks for the call this week. If that's okay with you I'll keep this PR in a draft state, just so we have easy access to this first code iteration and discussion.

[danielhollas]

I've rebased this PR on current master. The remaining changes to StructureManagerWidget are IMO quite modest. But we would still need to think about how to support structure editors.

In any case, this is blocked on solving #342 and subsequently implementing TrajectoryDataViewer.

CC @yakutovicha

[danielhollas]

Going to close this for now, since this is out of date, and I think a proper support for TrajectoryData will require more than is in this PR.