Skip to content
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

Polydisperse parameter check on model load #2553

Merged

Conversation

rozyczko
Copy link
Member

This adds a simple check for polydisperse parameters for the chosen model.
If there are such parameters, the polydisp checkbox and the polydisp tab are enabled.

Fixes #2551

How Has This Been Tested?

Local tests on windows

Review Checklist (please remove items if they don't apply):

  • Code has been reviewed
  • Functionality has been tested
  • Windows installer (GH artifact) has been tested (installed and worked)
  • MacOSX installer (GH artifact) has been tested (installed and worked)

Copy link
Contributor

@krzywon krzywon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good. This is an improvement over what already existed, so I am approving.

Potential improvements (but feel free to ignore any/all of these):

  1. If PD is checked and the model is changed, the checkbox remains checked. The tab is no longer enabled if no PD parameters are in the model, so it's mostly a visual issue, but unchecking the box wouldn't hurt.
  2. If no model is selected, the PD checkbox and tab are still enabled. Should this code be put into a class method that is run on tab creation, when the category changes, when the model changes, and (possibly...) when the structure factor changes? Adding a check if a model is selected to the front of has_poly should encompass all cases.

@rozyczko
Copy link
Member Author

  1. If PD is checked and the model is changed, the checkbox remains checked. The tab is no longer enabled if no PD parameters are in the model, so it's mostly a visual issue, but unchecking the box wouldn't hurt.

Unchecking the disabled checkbox means modifying user chosen state. When the user switches back to the previous model with PD params, they expect the checkbox to remain checked. Seeing the checked status go away and come back is, I think, even more confusing (not to mention, a nightmare to track).

  1. If no model is selected, the PD checkbox and tab are still enabled. Should this code be put into a class method that is run on tab creation, when the category changes, when the model changes, and (possibly...) when the structure factor changes? Adding a check if a model is selected to the front of has_poly should encompass all cases.

It is enough to make the check on the model change. I changed the initial state of this checkbox to false so the tab isn't active by default, just like magnetic.

@krzywon
Copy link
Contributor

krzywon commented Jul 14, 2023

It is enough to make the check on the model change. I changed the initial state of this checkbox to false so the tab isn't active by default, just like magnetic.

There is one use case this breaks down that I can think of. A user selects a model with polydisperse parameters, then selects a new category. The model box goes blank, but the polydispersity checkbox and tab remain enabled. It's a small issue so I don't think it should hold up this PR.

@butlerpd butlerpd added the Discuss At The Call Issues to be discussed at the fortnightly call label Jul 16, 2023
@butlerpd butlerpd merged commit 90735cf into main Jul 18, 2023
@rozyczko rozyczko deleted the 2551-polydispersity-and-models-without-disperse-parameters branch July 29, 2023 21:27
@butlerpd butlerpd removed the Discuss At The Call Issues to be discussed at the fortnightly call label Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Polydispersity and models without disperse parameters
3 participants