Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Implement viewer representations #373
Implement viewer representations #373
Changes from 70 commits
e3ce568
6768950
445a3aa
06a6f12
9cea518
1ea7067
5a12592
f652396
469ef53
9749526
4670934
83cbc81
c8006b6
3d642d1
090b94a
8467ce6
745e6cb
e0f5318
a04e32c
755a6a4
3ae05c3
2957cef
f3d6310
861c45d
1a1ce85
bf627bb
c73fc68
8e86ecb
fec6d9a
9c19ecb
4a51a95
61575a3
0e19bcc
a32d8b5
09aff81
d3a7cb5
2e26732
e7bef5e
fc4985d
ae7d8ad
fbc5a42
79ac26f
227d09b
bdda60d
85dcd83
3b0fdd1
616a727
76bc8e2
bd4e78c
0ab456d
d5c8320
bb3a536
44e36a2
5f190ec
b0521eb
5ac5e79
2d2efee
263d966
8f0559d
8746939
43a6776
ec9b553
38d19d3
f033582
c08712e
37639fb
1c1bc67
81ae8a9
8bb2229
13f9254
42b1d64
c38925b
2be1f30
374128a
5c82e8a
9592cb3
3b4843b
9424bb7
114959a
781684f
58f6a37
e75402f
133167f
b6c84e1
255564e
694cf47
523e207
f533787
23f9a2f
66ccfbd
6be5d98
cb6e6bb
f6ef4d1
5abbf5d
ed68daa
0543067
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an important (and breaking) change, can you explain why this was needed? I think we've discussed this a bit when I was trying to merge the TrajectoryViewer but don't remember details. Are we sure this does not break anything?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was a bug to link
structure_node
. Thestructure
trait is supposed to contain the current state of things, whilestructure_node
is an unstored AiiDA object.In the previous (buggy) version the AiiDA node would then be converted back to an ASE object losing loads of information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, but I think there were some design decisions involved in that. I think some of the traitlets were meant to be immutable, while the conversion to ASE was for the editor? I am not sure. No action needed if everything works (has the BasicEditor been tested?). I will take a closer look when I have time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it is a good idea to make the
structure_node
trait immutable because it is not meant to be modified externally. Its job is literally to be the StructureNode object corresponding to thestructure
trait (ASE).And, again, it was a mistake (probably mine) to link the
structure_node
trait instead of thestructure
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
structure_node
trait is already read-only so I think we're good here. Thanks for the explanation. I'll leave this unresolved just to keep this discussion visible.