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
Feature/handling molecules #834
Feature/handling molecules #834
Changes from 11 commits
d7986ed
0ed98f0
3f4d1ae
babed8a
517fc29
081d637
75a9e32
ce1f927
3097a79
2135569
0246168
894d0e3
e7e7026
c56a3b3
e302867
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.
I suggest not creating the link; instead, modify the
_override_changed
method. In this way, you only need to keep the logic in one place.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.
I need this link to be created and removed , so that the kpoints distance is not modify for molecules , while keeping the same logic for other periodicities
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.
since is liked to the reset()
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.
I understand that the link is used to disable the
kpoints_distance
widget. You can achieve this in_override_changed
. If input_structure is a molecule, thekpoints_distance
widget is disabled no matter the change is.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.
but the issue, is when uploading a job with pbc that is not molecule , so I tried with the override ,but i realize that since is linked with the reset , it was better to create and remove the link ,
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.
In my rework, I generally reserve
_on_some_state_change
methods as handlers of observations, as you do here. But this also means they themselves do not handle logic. So everything below the method header (including the docstring) moves to a more specific method, here maybe_update_relax_type
, passing to ischange["new"]
(orchange
if more than"new"
is required). This keeps the "controller"_on
methods light and clear. Reading them effectively tells you what is happening in the app (e.g., oninput_structure
change). Note that in my design, such methods as_update_relax_type
end up moving to the model. The controller triggers this so-called "business logic" via the model, keeping the controller light and dedicated only to reactions.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.
Just giving you a heads up of what to expect in my beast PR, so no worries on changing it here 🙂
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.
Yeah, I know! that part of removing was something that i wanted to check with you , so for molecules we cannot allow the user to do Cell Optimization , however in ipywidgets the ToogleButtons , there is no way to disable one option .. so I opted to go my way around and remove that option for molecules, while making sure it should be present for 1d,2d, and 3d systems.
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.
Right, I've encountered the limitation before. Would be nice in general if any option-selection widget allowed disabling certain options. Oh well...
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.
So , i would rename as
@tl.observe("input_structure")
def _update_relax_type(self, change):
or keep the name of _on_input_structure_change name , since i need the traitlets to trigger this function ?
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.
Again, I wouldn't change it at this stage. But if you did...
Sometimes, the triggered handler (
_on_<state>_change
) might trigger several methods. This pattern makes it clear what does are. For example:Easier to maintain. Also,
this
,that
, etc. are effectively "business logic", so they are moved to the model. Then you getException to the rule: controllers can also modify the view directly. In such a case, the method would not go through the model.
Please don't let any of the above block your PR 🙏
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.
ok i will keep the name , and i guess later we sit together for lazy loading :D