Conversation
|
Hey @anmolbhatia05, while reviewing your PR, I'd suggest the following code changes: ♻️ Refactored code for better testability and readability 🧪
You can also review and apply these suggestions locally on your machine. Learn more about GitKraken Code Suggest
Join your team on GitKraken to speed up PR review. |
|
The above comment was (mostly) auto generated using GitKraken. I was trying out a new feature (code suggest) to generate and send code suggestions. 😄 You can watch a video about it over on YT. |
- Refactored several functions to improve readability and maintainability - Replaced os module with pathlib for file operations - Improved docstrings for clarity and consistency and satify linters - Renamed 'widget' to '_widget' in widget.py to facilitate unit testing - Added a new test case for the _simulate function in widget.py -
There was a problem hiding this comment.
At a minimum please remove the .DS_store and add it to the .gitignore.
THe following could also be done in a follow up PR/-s:
I think using anywidget isn't really needed for this project (it doesn't need custom functionality that can only be achieved by JS) and you will have it easier using existing panel widgets.
The advantage is that those widgets are already decently styled using google material design and they already have nice convenience functionality (e.g. for a FloatInput, you can define min, max values and step size).
Also, you can use python to do the layouting (e.g. Row and Column, as well as tabs and don't need to deal with the frontend side of things).
Also instead of always generating files I would just keep the strings/xr.Dataset in memory and only save on user request.
And finally I would combine multiple widgets into a logical group that returns your pydantic models as state.
Here is an example of a SpectralWidget with all needed inputs and a SpectralWidgetManager that allows adding and removing those (PoC).
import panel as pn
import param
pn.extension('dark')
# Define the SpectralWidget class
class SpectralWidget:
def __init__(self, *, amplitude=0, location=0, width=1, skewness=0.1):
# Create individual widgets
self.amplitude_input = pn.widgets.FloatInput(name="Amplitude", value=amplitude, step=0.1)
self.location_input = pn.widgets.FloatInput(name="Location", value=location, step=0.1)
self.width_input = pn.widgets.FloatInput(name="Width", value=width, step=0.1)
self.skewness_input = pn.widgets.FloatInput(name="Skewness", value=skewness, step=0.1)
self.layout = pn.Row(
self.amplitude_input, self.location_input, self.width_input, self.skewness_input
)
@property
def amplitude(self):
return self.amplitude_input.value
@property
def location(self):
return self.location_input.value
@property
def width(self):
return self.width_input.value
@property
def skewness(self):
return self.skewness_input.value
def view(self):
return self.layout
# Create an instance of the custom class
spectral_widget = SpectralWidget()
# Display the combined widget
spectral_widget.view()import panel as pn
import param
from pyparamgui.schema import SpectralParameters
# Define the SpectralWidgetManager class
class SpectralWidgetManager(param.Parameterized):
def __init__(self, **params):
super().__init__(**params)
self.spectral_widgets = []
self.widgets_container = pn.Column()
# Create Add and Remove buttons
self.add_button = pn.widgets.Button(name='Add SpectralWidget', button_type='primary')
self.remove_button = pn.widgets.Button(name='Remove SpectralWidget', button_type='danger')
# Link buttons to methods
self.add_button.on_click(self.add_spectral_widget)
self.remove_button.on_click(self.remove_spectral_widget)
# Combine buttons and container into a Column
self.layout = pn.Column(
pn.Row(self.add_button, self.remove_button),
self.widgets_container
)
def add_spectral_widget(self, event):
new_widget = SpectralWidget()
self.spectral_widgets.append(new_widget)
self.widgets_container.append(new_widget.view())
def remove_spectral_widget(self, event):
if self.spectral_widgets:
widget_to_remove = self.spectral_widgets.pop()
self.widgets_container.remove(widget_to_remove.view())
def get_state(self):
state = SpectralParameters(amplitude=[],location=[],width=[],skewness=[])
for spectral_widget in self.spectral_widgets:
state.amplitude.append(spectral_widget.amplitude)
state.location.append(spectral_widget.location)
state.width.append(spectral_widget.width)
state.skewness.append(spectral_widget.skewness)
return state
def view(self):
return self.layout
# Create an instance of the manager class
spectral_widget_manager = SpectralWidgetManager()
# Display the manager widget
spectral_widget_manager.view()|
|
||
|
|
||
| @pytest.fixture() | ||
| def temp_dir(): |
There was a problem hiding this comment.
Pytest has a builtin tmp_path fixture that creates a temp folder per tests function.
There was a problem hiding this comment.
Good point. Mea culpa.
Initially I wanted to first have something I could easily run from main and fixtures don't play nice there, and then just copy pasted the code instead of switching to the dedicated fixture for that. 😄
s-weigand
left a comment
There was a problem hiding this comment.
As discussed with @jsnel we will merge this PR as is (besides some minor cleanups not related to the code itself) and make an MVP release since this was used in the master thesis of @anmolbhatia05.
After that we will take over working on and maintaining the project.
Thanks again @anmolbhatia05 for the MVP implementation of PyParamGUI as well as the other contribution to the pyglotaran ecosystem during your thesis. ❤️
This MVP is made using -
Please provide enough information so that others can review your pull request:
Explain the details for making this change. What existing problem does the pull request solve?.
Change summary
Checklist
Closes issues
closes #4