From 3f86e092a424ef6ae93ab1bbff0909ba721b2f20 Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Thu, 13 Jun 2024 10:02:53 -0500 Subject: [PATCH 01/22] Draft of tests for ReviewSettings --- .../settings/tests/test_custom_widgets.py | 86 +++++++++++++++++++ 1 file changed, 86 insertions(+) diff --git a/stellarphot/settings/tests/test_custom_widgets.py b/stellarphot/settings/tests/test_custom_widgets.py index c5dd3c32..3220cb21 100644 --- a/stellarphot/settings/tests/test_custom_widgets.py +++ b/stellarphot/settings/tests/test_custom_widgets.py @@ -1,24 +1,33 @@ +import os + import ipywidgets as ipw import pytest +from camel_converter import to_snake from ipyautoui.custom.iterable import ItemBox, ItemControl from stellarphot.settings import ( Camera, Observatory, PassbandMap, + PhotometryApertures, + PhotometryOptionalSettings, + PhotometryWorkingDirSettings, SavedSettings, + SourceLocationSettings, settings_files, ui_generator, ) from stellarphot.settings.custom_widgets import ( ChooseOrMakeNew, Confirm, + ReviewSettings, SaveStatus, SettingWithTitle, ) from stellarphot.settings.tests.test_models import ( DEFAULT_OBSERVATORY_SETTINGS, DEFAULT_PASSBAND_MAP, + DEFAULT_PHOTOMETRY_SETTINGS, TEST_CAMERA_VALUES, ) @@ -857,3 +866,80 @@ def test_title_decoration(self): # the saved indication. camera.savebuttonbar.bn_save.click() assert SaveStatus.SETTING_IS_SAVED in camera_title.title.value + + +SETTING_CLASSES = [ + Camera, + Observatory, + PassbandMap, + PhotometryApertures, + PhotometryOptionalSettings, + SourceLocationSettings, +] + + +def _to_space(name: str) -> str: + return name.replace("_", " ") + + +class TestReviewSettings: + """ + Test of the magical ReviewSettings widget. + """ + + # See test_settings_file.TestSavedSettings for a detailed description of what the + # following fixture does. In brief, it patches the settings_files.PlatformDirs class + # so that the user_data_dir method returns the temporary directory. + @pytest.fixture(autouse=True) + def fake_settings_dir(self, mocker, tmp_path): + mocker.patch.object( + settings_files.PlatformDirs, "user_data_dir", tmp_path / "stellarphot" + ) + + # This auto-used fixture changes the working directory to the temporary directory + # and then changes back to the original directory after the test is done. + @pytest.fixture(autouse=True) + def change_to_tmp_dir(self, tmp_path): + original_dir = os.getcwd() + os.chdir(tmp_path) + # Yielding here is important. It means that when the test is done, the remainder + # of the function will be executed. This is important because the test is run in + # a temporary directory and we want to change back to the original directory + # when the test is done. + yield + os.chdir(original_dir) + + @pytest.mark.parametrize("container_type", ["tabs", "accordion"]) + def test_creation_no_saved_settings(self, container_type): + # Check creation and names of tab when there are no saved settings + # and just one type of setting. + for setting_class in SETTING_CLASSES: + review_settings = ReviewSettings([setting_class], style=container_type) + assert review_settings._container.titles[0] == _to_space( + to_snake(setting_class.__name__) + ) + if container_type == "tabs": + assert isinstance(review_settings._container, ipw.Tab) + else: + assert isinstance(review_settings._container, ipw.Accordion) + + wd_settings = PhotometryWorkingDirSettings() + with pytest.raises( + ValueError, + match="Settings file photometry_settings.json does not exist", + ): + wd_settings.load() + + def test_creation_with_saved_settings(self): + # Check creation and names of tab when there are saved settings + # and just one type of setting. + + saveable_types = ChooseOrMakeNew._known_types + for setting_class in SETTING_CLASSES: + if setting_class.__name__ not in saveable_types: + continue + saved = SavedSettings() + item = DEFAULT_PHOTOMETRY_SETTINGS[to_snake(setting_class.__name__)] + saved.add_item(item) + review_settings = ReviewSettings([setting_class]) + assert review_settings._container.children[0].value == item From e77feebab54b47941e26eea7105407ea70afda0f Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Thu, 13 Jun 2024 10:01:38 -0500 Subject: [PATCH 02/22] Refactor title decoration and add draft review widget Handle StrEnum for Python 3.10 Add some explanation to ReviewSettings docstring --- pyproject.toml | 1 + stellarphot/settings/custom_widgets.py | 93 ++++++++++++++++++++++++++ 2 files changed, 94 insertions(+) diff --git a/pyproject.toml b/pyproject.toml index c04fc239..54df45a4 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -17,6 +17,7 @@ dependencies = [ "astroquery", "astrowidgets", "bottleneck", + "camel-converter", "ccdproc", "ginga", "ipyautoui >=0.7.15", diff --git a/stellarphot/settings/custom_widgets.py b/stellarphot/settings/custom_widgets.py index c88e1944..9fdd88f3 100644 --- a/stellarphot/settings/custom_widgets.py +++ b/stellarphot/settings/custom_widgets.py @@ -15,13 +15,16 @@ class StrEnum(str, Enum): import ipywidgets as ipw import traitlets as tr +from camel_converter import to_snake from ipyautoui.autoobject import AutoObject from ipyautoui.custom.iterable import ItemBox from stellarphot.settings import ( Camera, Observatory, + PartialPhotometrySettings, PassbandMap, + PhotometryWorkingDirSettings, SavedSettings, ui_generator, ) @@ -723,3 +726,93 @@ def title_decorator(plain_title, autoui_widget, change=None): return f"{plain_title} {SaveStatus.SETTING_NOT_SAVED}" else: return f"{plain_title} {SaveStatus.SETTING_IS_SAVED}" + + +class ReviewSettings(ipw.VBox): + """ + Widget to preview the saved settings in the working directory. It displays one + tab or accordion for each type of setting being reviewed. + + This widget does a bunch of automatic saving and loading behind the scenes: + + 1. When the widget is created, it loads the settings from the working directory, if + there are any. Settings loaded this way are marked as "need review" to remind the + user they might want to take a look. + 2. When the widget is created, any of the saveable settings are set to the default + for that setting and then saved to the working directory, with the tab markked as + "needs review". + 2. When the user clicks the save button for a setting that displays one, + the settings are saved to the working directory settings. + 3. When the user selects a setting from the settings that have a dropdown, the + selected setting is saved. Currently those settings are Camera, Observatory, and + PassbandMap, but the definitive list is given in + `stellarphot.settings.ChooseOrMakeNew._known_types`. + 4. Creating a new one of those saveable settings also saves it to the working + directory settings. + + """ + + def __init__(self, settings: list[str], style="tabs", *args, **kwargs) -> None: + super().__init__(*args, **kwargs) + # Get a copy of whatever settings may have already been saved. + try: + self._current_settings = PhotometryWorkingDirSettings().load() + except ValueError: + self._current_settings = PartialPhotometrySettings() + + self._setting_widgets = [] + names = [] + for setting in settings: + if setting.__name__ in ChooseOrMakeNew._known_types: + widget = ChooseOrMakeNew(setting.__name__) + val_to_set = widget._choose_existing + else: + widget = ui_generator(setting) + val_to_set = widget + + _add_saving_to_widget(widget) + self._setting_widgets.append(widget) + name = to_snake(setting.__name__) + names.append(" ".join(name.split("_"))) + + # This will be either a valid object or None + saved_value = getattr(self._current_settings, name) + if saved_value is not None: + val_to_set.value = saved_value + + if style == "tabs": + self._container = ipw.Tab() + else: + self._container = ipw.Accordion() + self._container.children = self._setting_widgets + self._container.titles = names + + self.children = [self._container] + + +def _add_saving_to_widget(setting_widget): + """ + Add an observer to a widget that autosaves the settings for that widget to + the working directory. + + Parameters + ---------- + setting_widget : ChooseOrMakeNew + The widget to add the observer to. + """ + wd_settings = PhotometryWorkingDirSettings() + # self._item_widget.savebuttonbar.fns_onsave_add_action(self.save_wd) + name = "" + + def save_wd(_=None): + pps = PartialPhotometrySettings(**{name: setting_widget.value}) + wd_settings.save(pps, update=True) + + if hasattr(setting_widget, "_choose_existing"): + setting_widget._choose_existing.observe(save_wd, "value") + name = to_snake(setting_widget._item_type_name) + elif hasattr(setting_widget, "savebuttonbar"): + setting_widget.savebuttonbar.fns_onsave_add_action(save_wd) + name = to_snake(setting_widget.model.__name__) + else: + raise ValueError("WTF am i supposed to do?") From 3a154f8d6f647511e0e27a35bb4f95aae71fbb0a Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Wed, 26 Jun 2024 11:44:14 -0500 Subject: [PATCH 03/22] Refactor ChooseOrMakeNew and badges --- stellarphot/settings/custom_widgets.py | 299 +++++++++++++----- .../settings/tests/test_custom_widgets.py | 153 ++++++--- 2 files changed, 336 insertions(+), 116 deletions(-) diff --git a/stellarphot/settings/custom_widgets.py b/stellarphot/settings/custom_widgets.py index 9fdd88f3..5340fc09 100644 --- a/stellarphot/settings/custom_widgets.py +++ b/stellarphot/settings/custom_widgets.py @@ -18,6 +18,7 @@ class StrEnum(str, Enum): from camel_converter import to_snake from ipyautoui.autoobject import AutoObject from ipyautoui.custom.iterable import ItemBox +from pydantic import ValidationError from stellarphot.settings import ( Camera, @@ -86,7 +87,7 @@ def __init__(self, item_type_name, *arg, details_hideable=False, **kwargs): # Descriptive title self._title = ipw.HTML( - value=(f"

Choose a {self._display_name} " "or make a new one

") + value=(f"Choose a {self._display_name} " "or make a new one") ) self._choose_detail_container = ipw.HBox(layout={"width": DEFAULT_BUTTON_WIDTH}) @@ -155,6 +156,7 @@ def __init__(self, item_type_name, *arg, details_hideable=False, **kwargs): # Set the selection to the first choice if there is one self._choose_existing.value = self._choose_existing.options[0][1] + # An observer has not been set up yet, so manually call the handler if len(self._choose_existing.options) == 1: # There are no items, so we are making a new one self._handle_selection({"new": "none"}) @@ -244,10 +246,13 @@ def _construct_choices(self): existing_choices = [(k, v) for k, v in saved_items.as_dict.items()] existing_choices = sorted(existing_choices, key=lambda x: x[0].lower()) choices = existing_choices + [(f"Make new {self._display_name}", "none")] + # self._choose_existing.value = None # This sets the options but doesn't select any of them self._choose_existing.options = choices def _handle_selection(self, change): + if change["new"] is None: + return if change["new"] == "none": # We are making a new item... @@ -513,6 +518,10 @@ def confirm_handler(change): if change["new"] is not None: item = self._item_widget.model(**self._item_widget.value) if self._editing or self._making_new: + # We are done editing/making new regardless of + # the confirmation outcome + self._making_new = False + self._editing = False if change["new"]: # User has said yes to updating the item, which we do by # deleting the old one and adding the new one. @@ -521,28 +530,28 @@ def confirm_handler(change): # Rebuild the dropdown list self._construct_choices() # Select the edited item + # To make 100% sure the observer is triggered, we set the value + # to None first. + self._choose_existing.value = None self._choose_existing.value = item else: # User has said no to updating the item, so we just # act as though the user has selected this item. if self._editing: + # _handle_selection always gets the value from disk, not + # from the ui, so this resets to the correct value. + # To make 100% sure the observer is triggered, we set the + # value to None first. + self._choose_existing.value = None self._handle_selection({"new": item}) else: # Set the selection to the first choice if there is one + # To make 100% sure the observer is triggered, we set the + # value to None first. + self._choose_existing.value = None self._choose_existing.value = self._choose_existing.options[ 0 ][1] - if self._making_new: - if self._show_details_shown: - self._show_details_ui.layout.display = "flex" - self._show_details_ui.value = ( - self._show_details_cached_value - ) - - # We are done editing/making new regardless of - # the confirmation outcome - self._editing = False - self._making_new = False elif self._deleting: if change["new"]: @@ -643,6 +652,16 @@ def _handle_no(self, _): self.value = False +class SaveStatus(StrEnum): + """ + Class to define the symbols used to represent a save status. + """ + + SETTING_NOT_SAVED = "❗️" + SETTING_IS_SAVED = "✅" + SETTING_SHOULD_BE_REVIEWED = "🔆" + + class SettingWithTitle(ipw.VBox): """ Class that adds a title to a setting widget made by ipyautoui and @@ -658,74 +677,68 @@ class SettingWithTitle(ipw.VBox): The setting widget to be displayed. """ + badge = tr.UseEnum(SaveStatus, default=None, allow_none=True) + def __init__(self, plain_title, widget, header_level=2, *args, **kwargs): super().__init__(*args, **kwargs) self._header_level = header_level self._plain_title = plain_title self._widget = widget - self.title = ipw.HTML(value=self._format_title(plain_title)) - self.children = [self.title, self._widget] - # Set up an observer to update title decoration when the settings - # change. - self._widget.observe(self._title_observer, names="_value") - # Also update after the save button is clicked - self._widget.savebuttonbar.fns_onsave_add_action(self._title_observer) - def _title_observer(self, change=None): - """ - Observer for the title of the widget. - """ - self.title.value = self._format_title( - title_decorator(self._plain_title, self._widget, change) + if isinstance(widget, ChooseOrMakeNew): + self.title = self._widget._title + self.children = [self._widget] + observer = self._choose_existing_observer + self._widget._choose_existing.observe(observer, names="value") + self._autoui_widget = self._widget._item_widget + # In case a value gets set programmatically.... + # self._autoui_widget.observe(self._title_observer, names="_value") + else: + self.title = ipw.HTML() + self._format_title(None) + self.children = [self.title, self._widget] + # Set up an observer to update title decoration when the settings + # change. + observer = self._title_observer + # Also update after the save button is clicked + # self._widget.savebuttonbar.fns_onsave_add_action(self._title_observer) + self._autoui_widget = self._widget + self._autoui_widget.savebuttonbar.observe(observer, names="unsaved_changes") + + def _choose_existing_observer(self, _=None): + """ + Observer for the ChooseOrMakeNew widget. + """ + # Unless we are making a new item or editing an item then what is displayed + # is saved. + if not self._widget._making_new and not self._widget._editing: + self.badge = SaveStatus.SETTING_IS_SAVED + else: + self.badge = SaveStatus.SETTING_NOT_SAVED + + @tr.observe("badge") + def _format_title(self, _=None): + badge = self.badge or "" + badge = badge + " " if badge else "" + self.title.value = ( + f"{badge}{self._plain_title}" ) - def _format_title(self, decorated_title): - return f"{decorated_title}" - - -class SaveStatus(StrEnum): - """ - Class to define the symbols used to represent a save status. - """ - - SETTING_NOT_SAVED = "❗️" - SETTING_IS_SAVED = "✅" - SETTING_SHOULD_BE_REVIEWED = "🔆" - - -def title_decorator(plain_title, autoui_widget, change=None): - """ - Decorate the title based on whether the settings need to be saved. - - Parameters - ---------- + def decorate_title(self): + """ + Public interface for forcing a title update. + """ + self._format_title() - change : dict, optional - Change dictionary from a traitlets event. It is optional so that this - method can be called without a change dictionary. - """ - # Keep track of settings state -- if dirty is true, the settings - # need to be saved. - dirty = False - - # If we got here via a traitlets event then change is a dict with keys - # "new" and "old", check that case first. By checking explicitly for - # this case, we guarantee that we catch changes in value even if - # the button bar's unsaved_changes is still False. - try: - if change["new"] != change["old"]: - dirty = True - except (KeyError, TypeError): - dirty = False - - # The unsaved_changes attribute is not a traitlet, and it isn't clear when - # in the event handling it gets set. When not called from an event, though, - # this function can only used unsaved_changes to decide what the title - # should be. - if autoui_widget.savebuttonbar.unsaved_changes or dirty: - return f"{plain_title} {SaveStatus.SETTING_NOT_SAVED}" - else: - return f"{plain_title} {SaveStatus.SETTING_IS_SAVED}" + def _title_observer(self, change): + """ + Observer for the title of the widget. + """ + if change["new"]: + # i.e. unsaved_changes is True + self.badge = SaveStatus.SETTING_NOT_SAVED + else: + self.badge = SaveStatus.SETTING_IS_SAVED class ReviewSettings(ipw.VBox): @@ -752,7 +765,7 @@ class ReviewSettings(ipw.VBox): """ - def __init__(self, settings: list[str], style="tabs", *args, **kwargs) -> None: + def __init__(self, settings, style="tabs", *args, **kwargs) -> None: super().__init__(*args, **kwargs) # Get a copy of whatever settings may have already been saved. try: @@ -761,34 +774,152 @@ def __init__(self, settings: list[str], style="tabs", *args, **kwargs) -> None: self._current_settings = PartialPhotometrySettings() self._setting_widgets = [] - names = [] + self._plain_names = [] + self.badges = [] + + self._settings = settings + for setting in settings: + # Track whether we are using the ChooseOrMakeNew or not + is_choose_or_make_new = False if setting.__name__ in ChooseOrMakeNew._known_types: widget = ChooseOrMakeNew(setting.__name__) val_to_set = widget._choose_existing + is_choose_or_make_new = True else: widget = ui_generator(setting) val_to_set = widget _add_saving_to_widget(widget) - self._setting_widgets.append(widget) name = to_snake(setting.__name__) - names.append(" ".join(name.split("_"))) + plain_name = " ".join(name.split("_")) + self._plain_names.append(plain_name) + self._setting_widgets.append(SettingWithTitle(plain_name, widget)) - # This will be either a valid object or None + # This should be either a valid object or None saved_value = getattr(self._current_settings, name) if saved_value is not None: - val_to_set.value = saved_value + try: + val_to_set.value = saved_value + except tr.TraitError as e: + # It can happen, while testing, that a setting gets saved to a local + # directory but is no longer in the saved settings for Camera, etc. + # We cannot fix that here, so raise a clearer error. + raise ValueError( + f"The {name} setting saved in the working directory is not " + f"consistent with the list of {name} items that are saved " + "in your permanent settings. Please fix this manually." + ) from e + # Add symbol to title to indicate that the setting needs review + self.badges.append(SaveStatus.SETTING_SHOULD_BE_REVIEWED) + + elif is_choose_or_make_new: + if len(widget._choose_existing.options) > 1: + # There is also one already-saved choice, so we load it. + # If we are using the ChooseOrMakeNew widget, we need to set the + # value of the widget to the default item to trigger the save. + # To do that, first set the value to None and then set the value + # back to the default item. + default_item = val_to_set.value + val_to_set.value = None + val_to_set.value = default_item + self.badges.append(SaveStatus.SETTING_SHOULD_BE_REVIEWED) + else: + # This setting needs to be made, not reviewed + self.badges.append(SaveStatus.SETTING_NOT_SAVED) + else: + # We got here because there was not a setting saved in the working + # directory, and this is not a ChooseOrMakeNew, which might have + # settings saved at the user level. + + # Two possibilities: + # 1. The setting can be made from default values but needs to be + # reviewed. Status should be "needs review". + # 2. The setting cannot be made from default values. Status should be + # "not saved". + try: + val_to_set.model() + except ValidationError: + self.badges.append(SaveStatus.SETTING_NOT_SAVED) + else: + self.badges.append(SaveStatus.SETTING_SHOULD_BE_REVIEWED) + + # Check that everything is consistent.... + assert len(self.badges) == len(self._plain_names) if style == "tabs": self._container = ipw.Tab() else: self._container = ipw.Accordion() + self._container.children = self._setting_widgets - self._container.titles = names + self._container.titles = self._make_titles() self.children = [self._container] + # Set up an observer to run when a tab is selected + self._container.observe(self._observe_tab_selection, names="selected_index") + + # Set up observer for each of the widget badges + for idx, widget in enumerate(self._setting_widgets): + widget.observe(self._observe_badge_change(idx), names="badge") + + def _make_titles(self): + """ + Make titles from badges and plain titles. + """ + return [ + f"{badge} {plain}" + for badge, plain in zip(self.badges, self._plain_names, strict=True) + ] + + def _get_autoui_widget(self, widget): + """ + Get the autoui widget for a given control. + + Parameters + ---------- + + widget: AutoUi widget or ChooseOrMakeNew + The widget to get the autoui widget from. + """ + return widget._autoui_widget + + def _observe_tab_selection(self, change): + """ + Observer for the tab or accordion selection. + """ + # Once the user has clicked on the tab, the status badge for the + # tab should just be the badge for the widget it holds. + + # Manually call the title decorator for this tab + new_selected = change["new"] + + setting_badge = self._setting_widgets[new_selected].badge + self.badges[new_selected] = setting_badge if setting_badge is not None else "" + + self._container.titles = self._make_titles() + + def _observe_badge_change(self, index): + """ + Observer for the badge of a setting widget. + """ + + def observer(change): + self.badges[index] = change["new"] + + # This should only be called when a badge changes, so we can just + # update the titles. + self._container.titles = self._make_titles() + + return observer + + def _index_of_setting(self, setting): + """ + Get the index of a setting in the list of settings. + """ + return self._settings.index(setting) + def _add_saving_to_widget(setting_widget): """ @@ -801,11 +932,19 @@ def _add_saving_to_widget(setting_widget): The widget to add the observer to. """ wd_settings = PhotometryWorkingDirSettings() - # self._item_widget.savebuttonbar.fns_onsave_add_action(self.save_wd) + + # Define name here so that it is available in the save_wd function. Its + # value will be set in the if/elif block below. name = "" def save_wd(_=None): - pps = PartialPhotometrySettings(**{name: setting_widget.value}) + try: + pps = PartialPhotometrySettings(**{name: setting_widget.value}) + except ValidationError: + # This can happen while making a new item, or while in the process of + # editing one + return + # We have a validated setting so save it. wd_settings.save(pps, update=True) if hasattr(setting_widget, "_choose_existing"): diff --git a/stellarphot/settings/tests/test_custom_widgets.py b/stellarphot/settings/tests/test_custom_widgets.py index 3220cb21..26f0b6f1 100644 --- a/stellarphot/settings/tests/test_custom_widgets.py +++ b/stellarphot/settings/tests/test_custom_widgets.py @@ -32,20 +32,21 @@ ) +# See test_settings_file.TestSavedSettings for a detailed description of what the +# following fixture does. In brief, it patches the settings_files.PlatformDirs class +# so that the user_data_dir method returns the temporary directory. +@pytest.fixture(autouse=True) +def fake_settings_dir(mocker, tmp_path): + mocker.patch.object( + settings_files.PlatformDirs, "user_data_dir", tmp_path / "stellarphot" + ) + + class TestChooseOrMakeNew: """ Class for testing the ChooseOrMakeNew widget. """ - # See test_settings_file.TestSavedSettings for a detailed description of what the - # following fixture does. In brief, it patches the settings_files.PlatformDirs class - # so that the user_data_dir method returns the temporary directory. - @pytest.fixture(autouse=True) - def fake_settings_dir(self, mocker, tmp_path): - mocker.patch.object( - settings_files.PlatformDirs, "user_data_dir", tmp_path / "stellarphot" - ) - def make_test_camera(self): """ Make a camera with the default testing values and save it. This came @@ -825,15 +826,15 @@ def test_title_format(self): == f"{plain_title}" ) - def test_title_decoration(self): + def test_title_decoration_plain_autoui(self): # Check that the title has the correct decoration given the # state of the settings widget. # Make a camera ui camera = ui_generator(Camera) + plain_title = "I am a camera" # Make a SettingWithTitle with the camera - plain_title = "I am a camera" camera_title = SettingWithTitle(plain_title, camera) # At the moment the title should not be decorated @@ -841,30 +842,79 @@ def test_title_decoration(self): assert SaveStatus.SETTING_NOT_SAVED not in camera_title.title.value # There should also be no unsaved changes at the moment - assert not camera.savebuttonbar.unsaved_changes + assert not camera_title._autoui_widget.savebuttonbar.unsaved_changes # Manually set unsaved_changes then call the change handler, which should # add an indication that there are unsaved changes. - camera.savebuttonbar.unsaved_changes = True - camera_title._title_observer() + camera_title._autoui_widget.di_widgets["gain"].value = str( + 2 * TEST_CAMERA_VALUES["gain"] + ) + # camera_title._autoui_widget.savebuttonbar.unsaved_changes = True + # camera_title._title_observer() assert SaveStatus.SETTING_NOT_SAVED in camera_title.title.value + # # Go back to unsaved_changes being False + # camera_title._autoui_widget.savebuttonbar.unsaved_changes = False + + # # Manually call the change handler, which should add an indication that + # # saves have been done. + # camera_title._title_observer() + # Click save + # assert SaveStatus.SETTING_IS_SAVED in camera_title.title.value + # Now change the camera value, which should trigger the change handler + camera_title._autoui_widget.value = TEST_CAMERA_VALUES - # Go back to unsaved_changes being False - camera.savebuttonbar.unsaved_changes = False + assert camera_title._autoui_widget.savebuttonbar.unsaved_changes + assert SaveStatus.SETTING_NOT_SAVED in camera_title.title.value - # Manually call the change handler, which should add an indication that - # saves have been done. - camera_title._title_observer() + # Finally, click the save button and the title should be decorated with + # the saved indication. + camera_title._autoui_widget.savebuttonbar.bn_save.click() assert SaveStatus.SETTING_IS_SAVED in camera_title.title.value - # Now change the camera value, which should trigger the change handler - camera.value = TEST_CAMERA_VALUES + @pytest.mark.parametrize("accept_edits", [True, False]) + def test_title_decoration_choose_or_make_new_editing(self, accept_edits): + # Check that the title has the correct decoration given the + # state of the settings widget. + + # Make an item so there is something to select + saved_setting = SavedSettings() + saved_setting.add_item(Camera(**TEST_CAMERA_VALUES)) + + camera = ChooseOrMakeNew(Camera.__name__) + plain_title = camera._title.value + + # Make a SettingWithTitle with the camera + camera_title = SettingWithTitle(plain_title, camera) + + # At the moment the title should not be decorated + assert SaveStatus.SETTING_IS_SAVED not in camera_title.title.value + assert SaveStatus.SETTING_NOT_SAVED not in camera_title.title.value + + # There should also be no unsaved changes at the moment + assert not camera_title._autoui_widget.savebuttonbar.unsaved_changes + + # Click the edit button + camera_title._widget._edit_button.click() + + # Edit a value in the camera so the save button is enabled + new_gain = 2 * TEST_CAMERA_VALUES["gain"] + camera_title._widget._item_widget.di_widgets["gain"].value = str(new_gain) assert SaveStatus.SETTING_NOT_SAVED in camera_title.title.value - # Finally, click the save button and the title should be decorated with - # the saved indication. - camera.savebuttonbar.bn_save.click() + # Click the save button.... + camera_title._widget._item_widget.savebuttonbar.bn_save.click() + + # ...should still be unsaved pending confirmation... + assert SaveStatus.SETTING_NOT_SAVED in camera_title.title.value + + # ...now confirm or deny the save... + if accept_edits: + camera_title._widget._confirm_edit_delete._yes.click() + else: + camera_title._widget._confirm_edit_delete._no.click() + + # ...and the title should be decorated with the saved indication. assert SaveStatus.SETTING_IS_SAVED in camera_title.title.value @@ -887,15 +937,6 @@ class TestReviewSettings: Test of the magical ReviewSettings widget. """ - # See test_settings_file.TestSavedSettings for a detailed description of what the - # following fixture does. In brief, it patches the settings_files.PlatformDirs class - # so that the user_data_dir method returns the temporary directory. - @pytest.fixture(autouse=True) - def fake_settings_dir(self, mocker, tmp_path): - mocker.patch.object( - settings_files.PlatformDirs, "user_data_dir", tmp_path / "stellarphot" - ) - # This auto-used fixture changes the working directory to the temporary directory # and then changes back to the original directory after the test is done. @pytest.fixture(autouse=True) @@ -915,8 +956,8 @@ def test_creation_no_saved_settings(self, container_type): # and just one type of setting. for setting_class in SETTING_CLASSES: review_settings = ReviewSettings([setting_class], style=container_type) - assert review_settings._container.titles[0] == _to_space( - to_snake(setting_class.__name__) + assert review_settings._container.titles[0].startswith( + _to_space(to_snake(setting_class.__name__)) ) if container_type == "tabs": assert isinstance(review_settings._container, ipw.Tab) @@ -937,9 +978,49 @@ def test_creation_with_saved_settings(self): saveable_types = ChooseOrMakeNew._known_types for setting_class in SETTING_CLASSES: if setting_class.__name__ not in saveable_types: + # The rest of this only makes sense if the setting class is saveable continue + + # Make a setting saved = SavedSettings() - item = DEFAULT_PHOTOMETRY_SETTINGS[to_snake(setting_class.__name__)] + # Make an instance of the class + snake_name = to_snake(setting_class.__name__) + item = DEFAULT_PHOTOMETRY_SETTINGS[snake_name] + # Save the instance to saved settings file saved.add_item(item) + + # Make the review widget. It should auto-populate with the item with just + # created because that is the only item of that type. review_settings = ReviewSettings([setting_class]) assert review_settings._container.children[0].value == item + + # This setting was made automatically by the widget, so the title of the + # container should end with SaveStatus.SETTING_SHOULD_BE_REVIEWED + assert review_settings._container.titles[0].endswith( + SaveStatus.SETTING_SHOULD_BE_REVIEWED + ) + + # The item setting should also have been saved to the working directory + wd_settings = PhotometryWorkingDirSettings() + loaded_settings = wd_settings.load() + assert getattr(loaded_settings, snake_name) == item + + def test_selecting_tab_updates_title(self): + # Check that selecting a tab updates the title of the widget to end with the + # appropriate indicator. + # Save a camera and an observatory + saved = SavedSettings() + camera = Camera(**TEST_CAMERA_VALUES) + saved.add_item(camera) + observatory = Observatory(**DEFAULT_OBSERVATORY_SETTINGS) + saved.add_item(observatory) + + # Create the review widget + review_settings = ReviewSettings([Camera, Observatory]) + + # Select the Observatory tab + review_settings._container.selected_index = 1 + + assert review_settings._container.titles[1].endswith( + SaveStatus.SETTING_IS_SAVED + ) From 4061ae3db8cf6b866f66cc66e81d039e2a663bf0 Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Wed, 26 Jun 2024 11:41:19 -0500 Subject: [PATCH 04/22] Update tests to match updated classes --- stellarphot/settings/tests/test_custom_widgets.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/stellarphot/settings/tests/test_custom_widgets.py b/stellarphot/settings/tests/test_custom_widgets.py index 26f0b6f1..0e763d1f 100644 --- a/stellarphot/settings/tests/test_custom_widgets.py +++ b/stellarphot/settings/tests/test_custom_widgets.py @@ -956,8 +956,9 @@ def test_creation_no_saved_settings(self, container_type): # and just one type of setting. for setting_class in SETTING_CLASSES: review_settings = ReviewSettings([setting_class], style=container_type) - assert review_settings._container.titles[0].startswith( + assert ( _to_space(to_snake(setting_class.__name__)) + in review_settings._container.titles[0] ) if container_type == "tabs": assert isinstance(review_settings._container, ipw.Tab) @@ -992,12 +993,14 @@ def test_creation_with_saved_settings(self): # Make the review widget. It should auto-populate with the item with just # created because that is the only item of that type. review_settings = ReviewSettings([setting_class]) - assert review_settings._container.children[0].value == item + model_dict = review_settings._container.children[0]._autoui_widget.value + assert setting_class(**model_dict) == item # This setting was made automatically by the widget, so the title of the # container should end with SaveStatus.SETTING_SHOULD_BE_REVIEWED - assert review_settings._container.titles[0].endswith( + assert ( SaveStatus.SETTING_SHOULD_BE_REVIEWED + in review_settings._container.titles[0] ) # The item setting should also have been saved to the working directory @@ -1021,6 +1024,4 @@ def test_selecting_tab_updates_title(self): # Select the Observatory tab review_settings._container.selected_index = 1 - assert review_settings._container.titles[1].endswith( - SaveStatus.SETTING_IS_SAVED - ) + assert SaveStatus.SETTING_IS_SAVED in review_settings._container.titles[1] From 55bf455dd30559f99858311f8d456f2deceb002c Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Thu, 27 Jun 2024 13:46:21 -0500 Subject: [PATCH 05/22] Improve test coverage in ReviewSettings --- stellarphot/settings/custom_widgets.py | 27 ++---- .../settings/tests/test_custom_widgets.py | 84 +++++++++++++++++++ 2 files changed, 91 insertions(+), 20 deletions(-) diff --git a/stellarphot/settings/custom_widgets.py b/stellarphot/settings/custom_widgets.py index 5340fc09..9cd49685 100644 --- a/stellarphot/settings/custom_widgets.py +++ b/stellarphot/settings/custom_widgets.py @@ -798,6 +798,7 @@ def __init__(self, settings, style="tabs", *args, **kwargs) -> None: # This should be either a valid object or None saved_value = getattr(self._current_settings, name) + if saved_value is not None: try: val_to_set.value = saved_value @@ -839,7 +840,9 @@ def __init__(self, settings, style="tabs", *args, **kwargs) -> None: # "not saved". try: val_to_set.model() - except ValidationError: + except ValidationError: # pragma: no cover + # This should never happen with the code base as of 2024-06-27 but + # might in the future. self.badges.append(SaveStatus.SETTING_NOT_SAVED) else: self.badges.append(SaveStatus.SETTING_SHOULD_BE_REVIEWED) @@ -873,18 +876,6 @@ def _make_titles(self): for badge, plain in zip(self.badges, self._plain_names, strict=True) ] - def _get_autoui_widget(self, widget): - """ - Get the autoui widget for a given control. - - Parameters - ---------- - - widget: AutoUi widget or ChooseOrMakeNew - The widget to get the autoui widget from. - """ - return widget._autoui_widget - def _observe_tab_selection(self, change): """ Observer for the tab or accordion selection. @@ -914,12 +905,6 @@ def observer(change): return observer - def _index_of_setting(self, setting): - """ - Get the index of a setting in the list of settings. - """ - return self._settings.index(setting) - def _add_saving_to_widget(setting_widget): """ @@ -954,4 +939,6 @@ def save_wd(_=None): setting_widget.savebuttonbar.fns_onsave_add_action(save_wd) name = to_snake(setting_widget.model.__name__) else: - raise ValueError("WTF am i supposed to do?") + raise ValueError( + f"The widget {setting_widget} is not a recognized type of widget." + ) diff --git a/stellarphot/settings/tests/test_custom_widgets.py b/stellarphot/settings/tests/test_custom_widgets.py index 0e763d1f..29b75061 100644 --- a/stellarphot/settings/tests/test_custom_widgets.py +++ b/stellarphot/settings/tests/test_custom_widgets.py @@ -1,4 +1,5 @@ import os +from pathlib import Path import ipywidgets as ipw import pytest @@ -8,6 +9,7 @@ from stellarphot.settings import ( Camera, Observatory, + PartialPhotometrySettings, PassbandMap, PhotometryApertures, PhotometryOptionalSettings, @@ -23,6 +25,7 @@ ReviewSettings, SaveStatus, SettingWithTitle, + _add_saving_to_widget, ) from stellarphot.settings.tests.test_models import ( DEFAULT_OBSERVATORY_SETTINGS, @@ -1008,6 +1011,26 @@ def test_creation_with_saved_settings(self): loaded_settings = wd_settings.load() assert getattr(loaded_settings, snake_name) == item + def test_creation_with_saved_working_dir_settings(self): + # Check that when there is a saved item in the working directory + # the badge attached to the name is "needs review" + photometry_apertures = DEFAULT_PHOTOMETRY_SETTINGS["photometry_apertures"] + wk_dir = PhotometryWorkingDirSettings() + partial_settings = PartialPhotometrySettings( + photometry_apertures=photometry_apertures + ) + wk_dir.save(partial_settings) + + # Create the review widget + review_settings = ReviewSettings( + [PhotometryOptionalSettings, PhotometryApertures] + ) + + assert ( + SaveStatus.SETTING_SHOULD_BE_REVIEWED + in review_settings._container.titles[1] + ) + def test_selecting_tab_updates_title(self): # Check that selecting a tab updates the title of the widget to end with the # appropriate indicator. @@ -1025,3 +1048,64 @@ def test_selecting_tab_updates_title(self): review_settings._container.selected_index = 1 assert SaveStatus.SETTING_IS_SAVED in review_settings._container.titles[1] + + def test_conflict_between_saved_and_working_dir(self): + # Check that the expected error is raised if the value of a saved setting + # like a camera conflicts with the value of the same setting in the working + # directory. + + # Make a camera for the saved settings (i.e. those in user's settings directory) + saved = SavedSettings() + camera = Camera(**TEST_CAMERA_VALUES) + saved.add_item(camera) + + # Make a camera for the working directory with different gain than the first + # camera + wd_settings = PhotometryWorkingDirSettings() + wd_camera = Camera(**TEST_CAMERA_VALUES) + wd_camera.gain = 2 * wd_camera.gain + wd_settings.save(PartialPhotometrySettings(camera=wd_camera)) + + # Create the review widget and check that the error is raised + with pytest.raises( + ValueError, match="The camera setting saved in the working directory is not" + ): + ReviewSettings([Camera]) + + def test_error_when_setting_has_no_saved_or_default_setting(self): + # Make a review widget with a setting that has no saved or default setting + # like Camera and check that an error is raised. + + review_settings = ReviewSettings([Camera]) + + assert SaveStatus.SETTING_NOT_SAVED in review_settings._container.titles[0] + + def test_setting_selected_item_to_none_does_not_save(self): + # The intent of this is to test an edge case where the user selects an item + # from the dropdown and then selects "None" from the dropdown. The selected + # item should be saved, but there should not be another save when None is + # selected. + saved = SavedSettings() + camera = Camera(**TEST_CAMERA_VALUES) + saved.add_item(camera) + wd_settings = PhotometryWorkingDirSettings() + wd_set_path = Path(wd_settings.partial_settings_file) + review_settings = ReviewSettings([Camera]) + + # Get the modification time of the working directory settings file + mtime = wd_set_path.stat().st_mtime + + assert ( + review_settings._setting_widgets[0]._widget._choose_existing.value == camera + ) + review_settings._setting_widgets[0]._widget._choose_existing.value = "none" + + # Check that the working directory settings file has not been modified + assert wd_set_path.stat().st_mtime == mtime + + +def test_add_saving_with_unrecognized_widget(): + # Check that an error is raised if a widget is added to the saving list + # that is not recognized. + with pytest.raises(ValueError, match="is not a recognized type of widget"): + _add_saving_to_widget(ipw.Dropdown()) From 5a377b9a28dc16148e1b112f2a26a9e716403c83 Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Thu, 27 Jun 2024 15:42:27 -0500 Subject: [PATCH 06/22] Fix logic in handling confirmation logic and test The test was not testing the code branch I thought because of a change in the logic in ChooseOrMakeNew. --- stellarphot/settings/custom_widgets.py | 12 ++++++---- .../settings/tests/test_custom_widgets.py | 24 ++++++++++++------- 2 files changed, 23 insertions(+), 13 deletions(-) diff --git a/stellarphot/settings/custom_widgets.py b/stellarphot/settings/custom_widgets.py index 9cd49685..c2c6fcc6 100644 --- a/stellarphot/settings/custom_widgets.py +++ b/stellarphot/settings/custom_widgets.py @@ -254,7 +254,6 @@ def _handle_selection(self, change): if change["new"] is None: return if change["new"] == "none": - # We are making a new item... # Hide the edit button @@ -514,6 +513,7 @@ def confirm_handler(change): The second case is the reason most of the handler is wrapped in an if statement. """ + was_editing = self._editing # value of None means the widget has been reset to not answered if change["new"] is not None: item = self._item_widget.model(**self._item_widget.value) @@ -537,13 +537,15 @@ def confirm_handler(change): else: # User has said no to updating the item, so we just # act as though the user has selected this item. - if self._editing: - # _handle_selection always gets the value from disk, not - # from the ui, so this resets to the correct value. + if was_editing: + # The user has presumably changed the value in the UI, so + # get the correct value from disk. + item = self._get_item(item.name) + # To make 100% sure the observer is triggered, we set the # value to None first. self._choose_existing.value = None - self._handle_selection({"new": item}) + self._choose_existing.value = item else: # Set the selection to the first choice if there is one # To make 100% sure the observer is triggered, we set the diff --git a/stellarphot/settings/tests/test_custom_widgets.py b/stellarphot/settings/tests/test_custom_widgets.py index 29b75061..f7489cd6 100644 --- a/stellarphot/settings/tests/test_custom_widgets.py +++ b/stellarphot/settings/tests/test_custom_widgets.py @@ -58,6 +58,8 @@ def make_test_camera(self): saved = SavedSettings() camera = Camera(**TEST_CAMERA_VALUES) saved.add_item(camera) + # Return the camera that was saved in case the test wants to use it + return camera def test_creation_without_type_raises_error(self): # Should raise an error if no type is provided @@ -192,9 +194,21 @@ def test_edit_item_saved_after_confirm(self): def test_edit_item_not_saved_after_cancel(self): # Should not save the item after clicking the No button - self.make_test_camera() + camera = self.make_test_camera() + + # Make an additional test camera so that we the result is the *second* camera + # in the list, not the first. If it is the first the outcome is the same + # whether the user was editing or making a new camera. + saved = SavedSettings() + camera.name = "zzzz" + camera.name + saved.add_item(camera) choose_or_make_new = ChooseOrMakeNew("camera") + assert len(choose_or_make_new._choose_existing.options) == 3 + choose_or_make_new._choose_existing.value = camera + + assert camera == choose_or_make_new.value + # Simulate a click on the edit button... choose_or_make_new._edit_button.click() @@ -205,13 +219,7 @@ def test_edit_item_not_saved_after_cancel(self): choose_or_make_new._item_widget.savebuttonbar.bn_save.click() # Simulate a click on the cancel button... choose_or_make_new._confirm_edit_delete._no.click() - - saved = SavedSettings() - cameras = saved.get_items("camera") - assert ( - cameras.as_dict[TEST_CAMERA_VALUES["name"]].gain - == TEST_CAMERA_VALUES["gain"] - ) + assert camera == choose_or_make_new.value def test_selecting_make_new_as_selection_works(self): # Should allow the user to select "Make new" as a selection From b949e0993f4b08d7315813a5785e262aeda44e2d Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Thu, 27 Jun 2024 19:51:07 -0500 Subject: [PATCH 07/22] Fix ruff command in tox.ini --- tox.ini | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tox.ini b/tox.ini index 0e0cd531..0ca304ea 100644 --- a/tox.ini +++ b/tox.ini @@ -51,4 +51,4 @@ commands = skip_install = true description = Run lint checks with ruff deps = ruff -commands = ruff {toxinidir} +commands = ruff check {toxinidir} From 6f66ad5c6536d95154155e924c89a37cf41455ff Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Fri, 28 Jun 2024 09:56:09 -0500 Subject: [PATCH 08/22] Test for and fix a case where badges were not set properly --- stellarphot/settings/custom_widgets.py | 4 +++ .../settings/tests/test_custom_widgets.py | 34 +++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/stellarphot/settings/custom_widgets.py b/stellarphot/settings/custom_widgets.py index c2c6fcc6..4ed98524 100644 --- a/stellarphot/settings/custom_widgets.py +++ b/stellarphot/settings/custom_widgets.py @@ -803,6 +803,10 @@ def __init__(self, settings, style="tabs", *args, **kwargs) -> None: if saved_value is not None: try: + if is_choose_or_make_new: + # Set to None first to ensure there is a change in the value + # when we set it to saved_value. + val_to_set.value = None val_to_set.value = saved_value except tr.TraitError as e: # It can happen, while testing, that a setting gets saved to a local diff --git a/stellarphot/settings/tests/test_custom_widgets.py b/stellarphot/settings/tests/test_custom_widgets.py index f7489cd6..353ec95c 100644 --- a/stellarphot/settings/tests/test_custom_widgets.py +++ b/stellarphot/settings/tests/test_custom_widgets.py @@ -1111,6 +1111,40 @@ def test_setting_selected_item_to_none_does_not_save(self): # Check that the working directory settings file has not been modified assert wd_set_path.stat().st_mtime == mtime + def test_clicking_tab_with_already_saved_settings_updates_badge(self): + # Check that if there are already settings saved to the working directory + # and the user clicks on the tab, the badge is updated to reflect that the + # setting has been saved. + + # To set this up we need to save settings to the working directory AND to the + # saved user settings. + wd_settings = PhotometryWorkingDirSettings() + camera = Camera(**TEST_CAMERA_VALUES) + observatory = Observatory(**DEFAULT_OBSERVATORY_SETTINGS) + passbands = PassbandMap(**DEFAULT_PASSBAND_MAP) + wd_settings.save( + PartialPhotometrySettings( + camera=camera, observatory=observatory, passband_map=passbands + ) + ) + saved = SavedSettings() + saved.add_item(camera) + saved.add_item(observatory) + saved.add_item(passbands) + + # Make the review widget + review_settings = ReviewSettings([Camera, Observatory, PassbandMap]) + for title in review_settings._container.titles: + assert SaveStatus.SETTING_SHOULD_BE_REVIEWED in title + + # Click on each tab, starting with the last one, to make sure a change in + # the selected value happens to trigger the observers. + for i in range(2, -1, -1): + review_settings._container.selected_index = i + + for title in review_settings._container.titles: + assert SaveStatus.SETTING_IS_SAVED in title + def test_add_saving_with_unrecognized_widget(): # Check that an error is raised if a widget is added to the saving list From 6cb0b9af9cd32079201dddd4709216bf0f78d987 Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Sat, 29 Jun 2024 11:21:53 -0500 Subject: [PATCH 09/22] Allow full settings to be updated from partial --- stellarphot/settings/settings_files.py | 80 +++++++++++++++---- .../settings/tests/test_settings_file.py | 26 ++++-- 2 files changed, 85 insertions(+), 21 deletions(-) diff --git a/stellarphot/settings/settings_files.py b/stellarphot/settings/settings_files.py index 1a410957..1a8e1271 100644 --- a/stellarphot/settings/settings_files.py +++ b/stellarphot/settings/settings_files.py @@ -340,6 +340,40 @@ def _are_partial_actually_full(self, settings): else: return True + def _update_settings_from_partial(self, disk_settings, partial_settings): + """ + Update the settings on disk, which may be full or partial, with the partial + settings. + + Parameters + ---------- + disk_settings : PhotometrySettings or PartialPhotometrySettings + The settings on disk. + + partial_settings : PartialPhotometrySettings + The partial settings to update with. + + Returns + ------- + PhotometrySettings pr PartialPhotometrySettings + The updated settings. The return type is the same as the type + of disk_settings. + """ + # Grab a dict of the settings, only keeping values that are not + # None. Making it dict so that the update method can be used to + # merge the two sets of settings. + + passed_partial_settings = { + k: v for k, v in partial_settings.model_dump().items() if v is not None + } + + # The order matters here. Keys in the argument passed_partial_settings + # will overwrite the keys in disk)settings. + # Note that update works in-place. + disk_settings.update(passed_partial_settings) + + return disk_settings + def save(self, settings, update=False): """ Save the partial or full settings to the working directory. Note well that @@ -362,9 +396,13 @@ def save(self, settings, update=False): disk, then the settings from disk that are not in the new settings are added to the new settings. - This means that in the event that the settings in the argument have, say, + This also means that in the event that the settings in the argument have, say, a `~stellarphot.settings.Camera`, and the file on disk also has one, the one in the argument is the one that will be kept. + + Finally, if we are passed a partial setting and there is a full setting on disk, + then the partial settings are merged with the full settings, and the full + settings are saved. """ full_settings = False @@ -381,26 +419,38 @@ def save(self, settings, update=False): # subclass of PhotometrySettings. # Are there already full settings? - # If so, we can't save partial settings. if self._settings_file.exists(): - raise ValueError( - "Cannot save partial settings when full settings already exist." - ) + if not update: + # If so, we can't save partial settings if the update flag + # is False. + raise ValueError( + "Cannot save partial settings when full " + "settings already exist." + ) + else: + # Load the full settings and update them with the + # partial settings + disk_settings = self._settings.model_dump() + + disk_settings = self._update_settings_from_partial( + disk_settings, settings + ) + + disk_settings = PhotometrySettings.model_validate(disk_settings) + + settings = disk_settings + # Are we updating or replacing partial settings? if update and self._partial_settings is not None: - # Grab a dict of the settings, only keeping values that are not - # None. Making it dict so that the update method can be used to - # merge the two sets of settings. - passed_partial_settings = { - k: v for k, v in settings.model_dump().items() if v is not None - } + # Get the partial settings that were loaded from disk existing_partial_settings = self._partial_settings.model_dump() - # The order matters here. Keys in the argument version of the - # settings will overwrite the keys in the file version of the - # settings. Note that update works in-place. - existing_partial_settings.update(passed_partial_settings) + # Update the partial settings with the new settings + existing_partial_settings = self._update_settings_from_partial( + existing_partial_settings, settings + ) + # Validate the updated partial settings settings = PartialPhotometrySettings.model_validate( existing_partial_settings ) diff --git a/stellarphot/settings/tests/test_settings_file.py b/stellarphot/settings/tests/test_settings_file.py index ff2bbebf..ebea8167 100644 --- a/stellarphot/settings/tests/test_settings_file.py +++ b/stellarphot/settings/tests/test_settings_file.py @@ -431,7 +431,8 @@ def test_save_partial_then_full_settings(self): assert settings_file.settings == full_settings assert settings_file.partial_settings is None - def test_save_full_then_partial_settings(self): + @pytest.mark.parametrize("update", [True, False]) + def test_save_full_then_partial_settings(self, update): # Test that saving full settings and then partial settings generates # the expected error. settings_file = PhotometryWorkingDirSettings() @@ -441,15 +442,28 @@ def test_save_full_then_partial_settings(self): assert not settings_file.partial_settings_file.exists() assert settings_file.settings == full_settings - partial_settings = PartialPhotometrySettings() - error_message = "Cannot save partial settings when full settings already exist" - with pytest.raises(ValueError, match=error_message): - settings_file.save(partial_settings) + camera = Camera.model_validate_json(CAMERA) + # Change the camera name we we can detect whether the setting saved + # to the working directory has been updated. + camera.name = "new camera" + partial_settings = PartialPhotometrySettings(camera=camera) + + if update: + settings_file.save(partial_settings, update=update) + else: + error_message = ( + "Cannot save partial settings when full settings already exist" + ) + with pytest.raises(ValueError, match=error_message): + settings_file.save(partial_settings, update=update) assert not settings_file.partial_settings_file.exists() assert settings_file.settings_file.exists() assert settings_file.partial_settings is None - assert settings_file.settings == full_settings + if update: + assert settings_file.settings.camera.name == camera.name + else: + assert settings_file.settings.camera.name == full_settings.camera.name def test_load_conflicting_partial_and_full_settings(self): # Make a valid partial settings file and a valid full settings file From a07792e9ab6c50d0754687af7e6aed80cfa233d7 Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Sat, 29 Jun 2024 14:48:08 -0500 Subject: [PATCH 10/22] Fix saving and tab status for settings with default values --- stellarphot/settings/custom_widgets.py | 52 ++++++++++++- .../settings/tests/test_custom_widgets.py | 76 ++++++++++++++++++- 2 files changed, 120 insertions(+), 8 deletions(-) diff --git a/stellarphot/settings/custom_widgets.py b/stellarphot/settings/custom_widgets.py index 4ed98524..04a50e3b 100644 --- a/stellarphot/settings/custom_widgets.py +++ b/stellarphot/settings/custom_widgets.py @@ -734,7 +734,8 @@ def decorate_title(self): def _title_observer(self, change): """ - Observer for the title of the widget. + Observer for the title of the widget, triggered when unsaved_changes + changes. """ if change["new"]: # i.e. unsaved_changes is True @@ -851,6 +852,9 @@ def __init__(self, settings, style="tabs", *args, **kwargs) -> None: # might in the future. self.badges.append(SaveStatus.SETTING_NOT_SAVED) else: + # This setting can be made from default values, so we save it to the + # working directory. + val_to_set.savebuttonbar.bn_save.click() self.badges.append(SaveStatus.SETTING_SHOULD_BE_REVIEWED) # Check that everything is consistent.... @@ -882,18 +886,58 @@ def _make_titles(self): for badge, plain in zip(self.badges, self._plain_names, strict=True) ] + @property + def current_settings(self): + """ + The current settings in the widget. + """ + try: + self._current_settings = PhotometryWorkingDirSettings().load() + except ValueError: + self._current_settings = PartialPhotometrySettings() + + return self._current_settings + def _observe_tab_selection(self, change): """ Observer for the tab or accordion selection. """ # Once the user has clicked on the tab, the status badge for the - # tab should just be the badge for the widget it holds. + # tab should just be the badge for the widget it holds, if the + # widget has a badge. OTherwise, compare the widget value to the + # saved value to determine the badge. - # Manually call the title decorator for this tab + # Get the index new_selected = change["new"] setting_badge = self._setting_widgets[new_selected].badge - self.badges[new_selected] = setting_badge if setting_badge is not None else "" + if setting_badge is not None: + self.badges[new_selected] = setting_badge + else: + # Check whether the setting is saved or not + setting_widget = self._setting_widgets[new_selected] + + try: + snake_name = to_snake(setting_widget._autoui_widget.model.__name__) + print(snake_name) + disk_value = getattr(self.current_settings, snake_name) + except KeyError: + print("KEY KEY KEY KEY ERROR") + # The setting is not saved + setting_widget = SaveStatus.SETTING_NOT_SAVED + else: + print("ELSE ELSE ELSE ELSE ELSE") + print(f"{disk_value=}") + print(f"{setting_widget._widget.value=}") + print(f"{setting_widget._autoui_widget.value=}") + value_from_widget = setting_widget._autoui_widget.model.model_validate( + setting_widget._autoui_widget.value + ) + print(f"{value_from_widget=}") + if disk_value == value_from_widget: + setting_widget.badge = SaveStatus.SETTING_IS_SAVED + else: + setting_widget.badge = SaveStatus.SETTING_SHOULD_BE_REVIEWED self._container.titles = self._make_titles() diff --git a/stellarphot/settings/tests/test_custom_widgets.py b/stellarphot/settings/tests/test_custom_widgets.py index 353ec95c..cae167f0 100644 --- a/stellarphot/settings/tests/test_custom_widgets.py +++ b/stellarphot/settings/tests/test_custom_widgets.py @@ -5,9 +5,11 @@ import pytest from camel_converter import to_snake from ipyautoui.custom.iterable import ItemBox, ItemControl +from pydantic import ValidationError from stellarphot.settings import ( Camera, + LoggingSettings, Observatory, PartialPhotometrySettings, PassbandMap, @@ -936,6 +938,7 @@ def test_title_decoration_choose_or_make_new_editing(self, accept_edits): PhotometryApertures, PhotometryOptionalSettings, SourceLocationSettings, + LoggingSettings, ] @@ -966,7 +969,9 @@ def test_creation_no_saved_settings(self, container_type): # Check creation and names of tab when there are no saved settings # and just one type of setting. for setting_class in SETTING_CLASSES: + review_settings = ReviewSettings([setting_class], style=container_type) + assert ( _to_space(to_snake(setting_class.__name__)) in review_settings._container.titles[0] @@ -977,11 +982,28 @@ def test_creation_no_saved_settings(self, container_type): assert isinstance(review_settings._container, ipw.Accordion) wd_settings = PhotometryWorkingDirSettings() - with pytest.raises( - ValueError, - match="Settings file photometry_settings.json does not exist", - ): + + # What happens next depends on whether the setting can be created from + # default values or not. + try: + setting_class() + except ValidationError: + created_from_defaults = False + else: + created_from_defaults = True + + if created_from_defaults: wd_settings.load() + snake_name = to_snake(setting_class.__name__) + assert ( + getattr(wd_settings.partial_settings, snake_name) == setting_class() + ) + else: + with pytest.raises( + ValueError, + match="Settings file photometry_settings.json does not exist", + ): + wd_settings.load() def test_creation_with_saved_settings(self): # Check creation and names of tab when there are saved settings @@ -1145,6 +1167,52 @@ def test_clicking_tab_with_already_saved_settings_updates_badge(self): for title in review_settings._container.titles: assert SaveStatus.SETTING_IS_SAVED in title + def test_clicking_tab_marks_green_when_all_saved(self): + # Check that if there are already settings saved to the working directory + # and the user clicks on the tab, the badge is updated to reflect that the + # setting has been saved. + + # To set this up we need to save settings to the working directory AND to the + # saved user settings. + wd_settings = PhotometryWorkingDirSettings() + camera = Camera(**TEST_CAMERA_VALUES) + observatory = Observatory(**DEFAULT_OBSERVATORY_SETTINGS) + passbands = PassbandMap(**DEFAULT_PASSBAND_MAP) + wd_settings.save( + PartialPhotometrySettings( + camera=camera, observatory=observatory, passband_map=passbands + ) + ) + saved = SavedSettings() + saved.add_item(camera) + saved.add_item(observatory) + saved.add_item(passbands) + + review_settings = ReviewSettings( + [ + Camera, + Observatory, + PassbandMap, + PhotometryApertures, + SourceLocationSettings, + PhotometryOptionalSettings, + LoggingSettings, + ] + ) + + num_tabs = len(review_settings._container.children) + + for title in review_settings._container.titles: + assert SaveStatus.SETTING_SHOULD_BE_REVIEWED in title + + # Click on each tab, starting with the last one, to make sure a change in + # the selected value happens to trigger the observers. + for i in range(num_tabs - 1, -1, -1): + review_settings._container.selected_index = i + + for title in review_settings._container.titles: + assert SaveStatus.SETTING_IS_SAVED in title + def test_add_saving_with_unrecognized_widget(): # Check that an error is raised if a widget is added to the saving list From 3e78c6c48b0e1b66e27c0d6c6ccb2d846a9c5057 Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Mon, 1 Jul 2024 16:21:13 -0500 Subject: [PATCH 11/22] Initial draft of review notebooks Rename notebooks to remove spaces --- .../notebooks/1-initial-settings.ipynb | 72 +++++++++++++++++++ .../4-final-review-of-settings.ipynb | 72 +++++++++++++++++++ 2 files changed, 144 insertions(+) create mode 100644 stellarphot/notebooks/1-initial-settings.ipynb create mode 100644 stellarphot/notebooks/4-final-review-of-settings.ipynb diff --git a/stellarphot/notebooks/1-initial-settings.ipynb b/stellarphot/notebooks/1-initial-settings.ipynb new file mode 100644 index 00000000..21c4a1ca --- /dev/null +++ b/stellarphot/notebooks/1-initial-settings.ipynb @@ -0,0 +1,72 @@ +{ + "cells": [ + { + "cell_type": "code", + "execution_count": null, + "id": "3a7f0b18-4509-474e-b42e-a69edd22913b", + "metadata": {}, + "outputs": [], + "source": [ + "from stellarphot.settings import Camera, Observatory, PassbandMap\n", + "from stellarphot.settings.custom_widgets import ReviewSettings" + ] + }, + { + "cell_type": "code", + "execution_count": null, + "id": "37494092-bd26-4dcb-a7d9-d587fd79ca45", + "metadata": {}, + "outputs": [], + "source": [ + "review = ReviewSettings([Camera, Observatory, PassbandMap])" + ] + }, + { + "cell_type": "markdown", + "id": "f25dc64e-e0b9-4673-b806-ef0e1f53f84c", + "metadata": {}, + "source": [ + "# Review or make key settings in the widget below " + ] + }, + { + "cell_type": "code", + "execution_count": null, + "id": "fe341afc-4171-4094-8165-82faa26706b3", + "metadata": {}, + "outputs": [], + "source": [ + "review" + ] + }, + { + "cell_type": "code", + "execution_count": null, + "id": "7c58cd05-afff-4273-8607-69a0e93f4fa4", + "metadata": {}, + "outputs": [], + "source": [] + } + ], + "metadata": { + "kernelspec": { + "display_name": "Python 3 (ipykernel)", + "language": "python", + "name": "python3" + }, + "language_info": { + "codemirror_mode": { + "name": "ipython", + "version": 3 + }, + "file_extension": ".py", + "mimetype": "text/x-python", + "name": "python", + "nbconvert_exporter": "python", + "pygments_lexer": "ipython3", + "version": "3.11.9" + } + }, + "nbformat": 4, + "nbformat_minor": 5 +} diff --git a/stellarphot/notebooks/4-final-review-of-settings.ipynb b/stellarphot/notebooks/4-final-review-of-settings.ipynb new file mode 100644 index 00000000..7b23cd95 --- /dev/null +++ b/stellarphot/notebooks/4-final-review-of-settings.ipynb @@ -0,0 +1,72 @@ +{ + "cells": [ + { + "cell_type": "code", + "execution_count": null, + "id": "3b91d12e-97ae-4d6d-970d-562b6bbfd683", + "metadata": {}, + "outputs": [], + "source": [ + "from stellarphot.settings import Camera, LoggingSettings, Observatory, PassbandMap, PhotometryApertures, PhotometryOptionalSettings, SourceLocationSettings\n", + "from stellarphot.settings.custom_widgets import ReviewSettings" + ] + }, + { + "cell_type": "code", + "execution_count": null, + "id": "d57c16cc-c243-47f3-9747-94d6c6898621", + "metadata": {}, + "outputs": [], + "source": [ + "review = ReviewSettings([Camera, Observatory, PassbandMap, PhotometryApertures, SourceLocationSettings, PhotometryOptionalSettings, LoggingSettings])" + ] + }, + { + "cell_type": "markdown", + "id": "36db3d1f-d0d7-413e-ab77-766c6e3d02a1", + "metadata": {}, + "source": [ + "# Review or make key settings in the widget below " + ] + }, + { + "cell_type": "code", + "execution_count": null, + "id": "39491574-cc9f-4d2b-963f-1b175336a16d", + "metadata": {}, + "outputs": [], + "source": [ + "review" + ] + }, + { + "cell_type": "code", + "execution_count": null, + "id": "629f505c-6fab-4bf9-a30a-130bf668d8ac", + "metadata": {}, + "outputs": [], + "source": [] + } + ], + "metadata": { + "kernelspec": { + "display_name": "Python 3 (ipykernel)", + "language": "python", + "name": "python3" + }, + "language_info": { + "codemirror_mode": { + "name": "ipython", + "version": 3 + }, + "file_extension": ".py", + "mimetype": "text/x-python", + "name": "python", + "nbconvert_exporter": "python", + "pygments_lexer": "ipython3", + "version": "3.11.9" + } + }, + "nbformat": 4, + "nbformat_minor": 5 +} From 97ebbf4029a0d706cf9b0ebab65c6bbcea423e4e Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Mon, 1 Jul 2024 16:00:07 -0500 Subject: [PATCH 12/22] Refactor setting FileChooser filename This code is necessary for now because of a bug in ipyautoui, #323 Refactor handling of FileChooser bug Again! This fix is much more sensible than the last one, though, and will require just removing a few lines of code in one place (views.py) once the bug is fixed. --- stellarphot/gui_tools/comparison_functions.py | 30 ++---------- .../settings/tests/test_custom_widgets.py | 41 ++++++++++++++++- stellarphot/settings/views.py | 46 ++++++++++++++++++- 3 files changed, 87 insertions(+), 30 deletions(-) diff --git a/stellarphot/gui_tools/comparison_functions.py b/stellarphot/gui_tools/comparison_functions.py index d468a2cb..cede47d8 100644 --- a/stellarphot/gui_tools/comparison_functions.py +++ b/stellarphot/gui_tools/comparison_functions.py @@ -266,8 +266,9 @@ def __init__( self.photometry_settings = PhotometryWorkingDirSettings() if photom_apertures_file is not None: - # Set the source location file name to the name passed in - self._set_source_location_file_to_value(photom_apertures_file) + original_settings = self.source_locations.value.copy() + original_settings["source_list_file"] = photom_apertures_file + self.source_locations.value = original_settings self.overwrite_outputs = overwrite_outputs @@ -469,30 +470,6 @@ def save(self): # Update the save box title to reflect the save self.source_and_title.decorate_title() - def _set_source_location_file_to_value(self, name=None): - # Right now the source location file name will not be set to the default name - # properly because of a bug in ipyautoui, so we set it manually here. - # Easier once/if https://github.com/maxfordham/ipyautoui/pull/323 is merged - # and released. Then we can just set the value of the widget directly like this: - # - # self.source_locations.value = {'source_list_file': name, ...rest of settings} - file_chooser = self.source_locations.di_widgets["source_list_file"] - if name is None: - # Use the default name - name = self.source_locations.model.model_fields["source_list_file"].default - # Make sure ipyautoui knows the value has changed - file_chooser.value = name - - # Set the value to what we actually want - file_chooser.reset(".", name) - file_chooser._apply_selection() - - # Because we have updated the value outside of ipyautoui, also force an update - # of the widget value. - current_value = self.source_locations.value.copy() - current_value["source_list_file"] = file_chooser.value - self.source_locations.value = current_value - def _viewer(self): header = ipw.HTML( value=""" @@ -529,7 +506,6 @@ def _viewer(self): SourceLocationSettings, max_field_width="75px" ) - self._set_source_location_file_to_value() self.source_and_title = SettingWithTitle( "Source location settings", self.source_locations ) diff --git a/stellarphot/settings/tests/test_custom_widgets.py b/stellarphot/settings/tests/test_custom_widgets.py index cae167f0..d3b32bf5 100644 --- a/stellarphot/settings/tests/test_custom_widgets.py +++ b/stellarphot/settings/tests/test_custom_widgets.py @@ -969,7 +969,13 @@ def test_creation_no_saved_settings(self, container_type): # Check creation and names of tab when there are no saved settings # and just one type of setting. for setting_class in SETTING_CLASSES: + wd_settings = PhotometryWorkingDirSettings() + # Remove any existing settings files that may have been saved in earlier + # iterations of the loop. + p = Path(".") + (p / wd_settings.partial_settings_file).unlink(missing_ok=True) + (p / wd_settings.settings_file).unlink(missing_ok=True) review_settings = ReviewSettings([setting_class], style=container_type) assert ( @@ -981,8 +987,6 @@ def test_creation_no_saved_settings(self, container_type): else: assert isinstance(review_settings._container, ipw.Accordion) - wd_settings = PhotometryWorkingDirSettings() - # What happens next depends on whether the setting can be created from # default values or not. try: @@ -1213,6 +1217,39 @@ def test_clicking_tab_marks_green_when_all_saved(self): for title in review_settings._container.titles: assert SaveStatus.SETTING_IS_SAVED in title + def test_loading_saved_source_locations_to_ui(self): + # Check that saved source locations are loaded into the UI + wd_set = PhotometryWorkingDirSettings() + source_locations = SourceLocationSettings() + wd_set.save( + PartialPhotometrySettings(source_location_settings=source_locations) + ) + + review_settings = ReviewSettings([SourceLocationSettings]) + + # Check that the saved source locations are in the UI + assert ( + review_settings._setting_widgets[0]._autoui_widget.value + == source_locations.model_dump() + ) + assert ( + review_settings._setting_widgets[0] + ._autoui_widget.di_widgets["source_list_file"] + .selected_filename + == source_locations.source_list_file + ) + + def test_getting_settings_with_nothing_saved(self): + # Check that when we create the object with no saved settings we get an empty + # partial settings object. + review_settings = ReviewSettings([Camera]) + assert review_settings.current_settings == PartialPhotometrySettings() + + def test_selecting_table_without_saved_setting_sets_proper_badge(self): + review_settings = ReviewSettings([PhotometryApertures, Camera]) + review_settings._container.selected_index = 1 + assert SaveStatus.SETTING_NOT_SAVED in review_settings._container.titles[1] + def test_add_saving_with_unrecognized_widget(): # Check that an error is raised if a widget is added to the saving list diff --git a/stellarphot/settings/views.py b/stellarphot/settings/views.py index c8cfd11c..a850a85a 100644 --- a/stellarphot/settings/views.py +++ b/stellarphot/settings/views.py @@ -1,7 +1,9 @@ +from pathlib import Path + from ipyautoui import AutoUi from ipywidgets import Layout -from .models import _extract_short_description +from .models import SourceLocationSettings, _extract_short_description __all__ = ["ui_generator"] @@ -30,6 +32,19 @@ def ui_generator(model, max_field_width=None, file_chooser_max_width=None): """ ui = AutoUi(model) + # Oof, there is a bug in the file choose from ipyautoui. We patch up the + # SourceLocationSettings widget here. + if model == SourceLocationSettings: + + # Set up the observer for future changes in the value of the widget + _file_chooser_value_fixer(ui) + + # Force a call to the handler to make sure the initial value is correct + name = model.model_fields["source_list_file"].default + value = ui.value.copy() + value["source_list_file"] = name + ui.value = value + # validation is really messy looking right now, so suppress display of # the validation errors. A green check or red x will still be displayed. ui.show_validation = False @@ -139,3 +154,32 @@ def handler(_): button.disabled = not needs_to_save return handler + + +def _file_chooser_value_fixer(source_locations_ui): + """ + Add an observer to the source_locations_ui widget to set the source_list_file + correctly in FileChooser UI. + + Parameters + ---------- + source_locations_ui : `ipyautoui.AutoUi` + The user interface widget for the source locations. + """ + file_chooser = source_locations_ui.di_widgets["source_list_file"] + + def handler(_): + """ + A handler must take an argument but we don't use it here. + """ + + if (Path(file_chooser.selected_path) / file_chooser.selected_filename) != Path( + file_chooser.value + ): + print("😱" * 10) + file_chooser.reset( + file_chooser.selected_path, Path(file_chooser.value).name + ) + file_chooser._apply_selection() + + source_locations_ui.observe(handler, "_value") From 18bb9211a4b3ef6e5796d93b1699bddf2771e32e Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Mon, 1 Jul 2024 16:15:57 -0500 Subject: [PATCH 13/22] Update logic when tab is selected Remove debugging print --- stellarphot/settings/custom_widgets.py | 19 ++++++------------- stellarphot/settings/views.py | 1 - 2 files changed, 6 insertions(+), 14 deletions(-) diff --git a/stellarphot/settings/custom_widgets.py b/stellarphot/settings/custom_widgets.py index 04a50e3b..dd3b8340 100644 --- a/stellarphot/settings/custom_widgets.py +++ b/stellarphot/settings/custom_widgets.py @@ -917,27 +917,20 @@ def _observe_tab_selection(self, change): # Check whether the setting is saved or not setting_widget = self._setting_widgets[new_selected] - try: - snake_name = to_snake(setting_widget._autoui_widget.model.__name__) - print(snake_name) - disk_value = getattr(self.current_settings, snake_name) - except KeyError: - print("KEY KEY KEY KEY ERROR") + snake_name = to_snake(setting_widget._autoui_widget.model.__name__) + + disk_value = getattr(self.current_settings, snake_name) + if disk_value is None: # The setting is not saved setting_widget = SaveStatus.SETTING_NOT_SAVED else: - print("ELSE ELSE ELSE ELSE ELSE") - print(f"{disk_value=}") - print(f"{setting_widget._widget.value=}") - print(f"{setting_widget._autoui_widget.value=}") + # Set the badge to saved if it has been saved. value_from_widget = setting_widget._autoui_widget.model.model_validate( setting_widget._autoui_widget.value ) - print(f"{value_from_widget=}") + if disk_value == value_from_widget: setting_widget.badge = SaveStatus.SETTING_IS_SAVED - else: - setting_widget.badge = SaveStatus.SETTING_SHOULD_BE_REVIEWED self._container.titles = self._make_titles() diff --git a/stellarphot/settings/views.py b/stellarphot/settings/views.py index a850a85a..18469305 100644 --- a/stellarphot/settings/views.py +++ b/stellarphot/settings/views.py @@ -176,7 +176,6 @@ def handler(_): if (Path(file_chooser.selected_path) / file_chooser.selected_filename) != Path( file_chooser.value ): - print("😱" * 10) file_chooser.reset( file_chooser.selected_path, Path(file_chooser.value).name ) From e716fa7805aa1cf282f3665b2f27832bbae0e12c Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Mon, 1 Jul 2024 16:00:29 -0500 Subject: [PATCH 14/22] Use a temporary settings directory in tests --- stellarphot/gui_tools/tests/test_seeing_profile.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/stellarphot/gui_tools/tests/test_seeing_profile.py b/stellarphot/gui_tools/tests/test_seeing_profile.py index 1253ea00..cec8f5e2 100644 --- a/stellarphot/gui_tools/tests/test_seeing_profile.py +++ b/stellarphot/gui_tools/tests/test_seeing_profile.py @@ -22,6 +22,7 @@ Observatory, PhotometryApertures, PhotometryWorkingDirSettings, + settings_files, # This import is needed for mocking ) from stellarphot.settings.tests.test_models import ( TEST_CAMERA_VALUES, @@ -64,6 +65,18 @@ def test_seeing_profile_object_creation(): assert isinstance(profile_widget.box, ipw.Box) +@pytest.fixture(autouse=True) +def fake_settings_dir(mocker, tmp_path): + # See test_settings_files.py for more information on this fixture. + # It makes a fake settings directory for each test to use. + + # stellarphot is added to the name of the directory to make sure we start + # without a stellarphot directory for each test. + mocker.patch.object( + settings_files.PlatformDirs, "user_data_dir", tmp_path / "stellarphot" + ) + + def test_seeing_profile_properties(tmp_path): # Here we make a seeing profile then load an image. profile_widget = spf.SeeingProfileWidget( From 88a8668bc370995d109a1c5f8fbb30d336c756b2 Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Mon, 1 Jul 2024 16:25:22 -0500 Subject: [PATCH 15/22] Add new notebooks to jupyterlab launcher --- .jp_app_launcher_stellarphot.yaml | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/.jp_app_launcher_stellarphot.yaml b/.jp_app_launcher_stellarphot.yaml index 8937515b..48877f27 100644 --- a/.jp_app_launcher_stellarphot.yaml +++ b/.jp_app_launcher_stellarphot.yaml @@ -1,12 +1,12 @@ -- title: Saveable settings for testing +- title: 1 - Saveable settings description: Settings for camera, observatory and passbands - source: ../stellarphot/saveable-settings.ipynb + source: ../stellarphot/1-initial-settings.ipynb icon: ../stellarphot/stellarphot-logo.svg cwd: not_used type: notebook - catalog: Stellarphot settings + catalog: Stellarphot photometry -- title: Seeing profile +- title: 2 - Seeing profile description: Choose aperture settings source: ../stellarphot/photometry/01-viewer-seeing-template.ipynb icon: ../stellarphot/stellarphot-logo.svg @@ -14,10 +14,18 @@ type: notebook catalog: Stellarphot photometry -- title: Comparison stars +- title: 3 - Comparison stars description: Choose comparison stars source: ../stellarphot/photometry/02-viewer-comparison-stars-template.ipynb icon: ../stellarphot/stellarphot-logo.svg cwd: not_used type: notebook catalog: Stellarphot photometry + +- title: 4 - Review settings + description: Choose comparison stars + source: ../stellarphot/photometry/4-final-review-of-settings.ipynb + icon: ../stellarphot/stellarphot-logo.svg + cwd: not_used + type: notebook + catalog: Stellarphot photometry From 082f46fd38800d042475202d255a48a44a15a9b2 Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Mon, 1 Jul 2024 16:27:50 -0500 Subject: [PATCH 16/22] Temporarily fix linting issue --- pyproject.toml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pyproject.toml b/pyproject.toml index 54df45a4..53b4badc 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -126,6 +126,8 @@ select = [ "__init__.py" = ["E402", "F403"] # Ignore F405 (variable may be from star imports) in docs/conf.py "docs/conf.py" = ["F405"] +# Ignore F811 (redefinition of unused) in core for the moment +"core.py" = ["F811"] [tool.codespell] skip = '*.svg' From fe1fd5f4ee2e655331f5366fb91787aece4aef7d Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Mon, 1 Jul 2024 16:50:47 -0500 Subject: [PATCH 17/22] Ignore a photutils warning for now Suppress warning again Fixes for photutils 1.13 --- pyproject.toml | 2 ++ stellarphot/photometry/tests/fake_image.py | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 53b4badc..57774442 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -176,4 +176,6 @@ filterwarnings = [ 'ignore:Deprecated in traitlets 4.1, use the instance:DeprecationWarning', # Some WCS headers are issuing warnings 'ignore:RADECSYS=:', + # photutils changed the name of a function again + 'ignore:The make_gaussian_sources_image function is deprecated', ] diff --git a/stellarphot/photometry/tests/fake_image.py b/stellarphot/photometry/tests/fake_image.py index 69283466..0301a7cf 100644 --- a/stellarphot/photometry/tests/fake_image.py +++ b/stellarphot/photometry/tests/fake_image.py @@ -29,7 +29,7 @@ def __init__(self, noise_dev=1.0, seed=None): self._sources = Table.read(data_file) self.mean_noise = self.sources["amplitude"].max() / 100 self.noise_dev = noise_dev - self._stars = make_gaussian_sources_image(self.image_shape, self.sources) + self._stars = make_gaussian_sources_image(tuple(self.image_shape), self.sources) self._noise = make_noise_image( self._stars.shape, mean=self.mean_noise, stddev=noise_dev, seed=seed ) @@ -194,7 +194,7 @@ def shift_FakeCCDImage(ccd_data, x_shift, y_shift): # Make image srcs = make_gaussian_sources_image( - shifted_ccd_data.image_shape, shifted_ccd_data.sources + tuple(shifted_ccd_data.image_shape), shifted_ccd_data.sources ) background = make_noise_image( srcs.shape, mean=shifted_ccd_data.mean_noise, stddev=shifted_ccd_data.noise_dev From a631eb31f1709e86a5bdb5027add8721d7697f98 Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Wed, 3 Jul 2024 09:38:51 -0500 Subject: [PATCH 18/22] Update app launcher configuration --- .jp_app_launcher_stellarphot.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.jp_app_launcher_stellarphot.yaml b/.jp_app_launcher_stellarphot.yaml index 48877f27..5c76be38 100644 --- a/.jp_app_launcher_stellarphot.yaml +++ b/.jp_app_launcher_stellarphot.yaml @@ -16,7 +16,7 @@ - title: 3 - Comparison stars description: Choose comparison stars - source: ../stellarphot/photometry/02-viewer-comparison-stars-template.ipynb + source: ../stellarphot/photometry/02-comp-star-plotter-template.ipynb icon: ../stellarphot/stellarphot-logo.svg cwd: not_used type: notebook @@ -24,7 +24,7 @@ - title: 4 - Review settings description: Choose comparison stars - source: ../stellarphot/photometry/4-final-review-of-settings.ipynb + source: ../stellarphot/4-final-review-of-settings.ipynb icon: ../stellarphot/stellarphot-logo.svg cwd: not_used type: notebook From d350c98eae948a569a11a83a597d41fde2b23427 Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Sun, 14 Jul 2024 17:50:52 -0700 Subject: [PATCH 19/22] Apply suggestions from code review by @JuanCab Co-authored-by: Juan Cabanela --- stellarphot/settings/custom_widgets.py | 2 +- stellarphot/settings/settings_files.py | 2 +- stellarphot/settings/tests/test_custom_widgets.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/stellarphot/settings/custom_widgets.py b/stellarphot/settings/custom_widgets.py index dd3b8340..b34e958b 100644 --- a/stellarphot/settings/custom_widgets.py +++ b/stellarphot/settings/custom_widgets.py @@ -904,7 +904,7 @@ def _observe_tab_selection(self, change): """ # Once the user has clicked on the tab, the status badge for the # tab should just be the badge for the widget it holds, if the - # widget has a badge. OTherwise, compare the widget value to the + # widget has a badge. Otherwise, compare the widget value to the # saved value to determine the badge. # Get the index diff --git a/stellarphot/settings/settings_files.py b/stellarphot/settings/settings_files.py index 1a8e1271..6a334ee1 100644 --- a/stellarphot/settings/settings_files.py +++ b/stellarphot/settings/settings_files.py @@ -368,7 +368,7 @@ def _update_settings_from_partial(self, disk_settings, partial_settings): } # The order matters here. Keys in the argument passed_partial_settings - # will overwrite the keys in disk)settings. + # will overwrite the keys in disk settings. # Note that update works in-place. disk_settings.update(passed_partial_settings) diff --git a/stellarphot/settings/tests/test_custom_widgets.py b/stellarphot/settings/tests/test_custom_widgets.py index d3b32bf5..ef07f54a 100644 --- a/stellarphot/settings/tests/test_custom_widgets.py +++ b/stellarphot/settings/tests/test_custom_widgets.py @@ -198,7 +198,7 @@ def test_edit_item_not_saved_after_cancel(self): # Should not save the item after clicking the No button camera = self.make_test_camera() - # Make an additional test camera so that we the result is the *second* camera + # Make an additional test camera so that the result is the *second* camera # in the list, not the first. If it is the first the outcome is the same # whether the user was editing or making a new camera. saved = SavedSettings() From 0b2054527b1e6de98e3c47dec856348a2077208f Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Sun, 14 Jul 2024 17:58:38 -0700 Subject: [PATCH 20/22] Clarify intent of test steps and remove cruft --- .../settings/tests/test_custom_widgets.py | 27 +++++++------------ 1 file changed, 10 insertions(+), 17 deletions(-) diff --git a/stellarphot/settings/tests/test_custom_widgets.py b/stellarphot/settings/tests/test_custom_widgets.py index ef07f54a..6d53cf0d 100644 --- a/stellarphot/settings/tests/test_custom_widgets.py +++ b/stellarphot/settings/tests/test_custom_widgets.py @@ -857,28 +857,21 @@ def test_title_decoration_plain_autoui(self): # There should also be no unsaved changes at the moment assert not camera_title._autoui_widget.savebuttonbar.unsaved_changes - # Manually set unsaved_changes then call the change handler, which should - # add an indication that there are unsaved changes. + # Manually set the value of the gain, which should make the trait + # unsaved_changes True... camera_title._autoui_widget.di_widgets["gain"].value = str( 2 * TEST_CAMERA_VALUES["gain"] ) - # camera_title._autoui_widget.savebuttonbar.unsaved_changes = True - # camera_title._title_observer() - assert SaveStatus.SETTING_NOT_SAVED in camera_title.title.value - # # Go back to unsaved_changes being False - # camera_title._autoui_widget.savebuttonbar.unsaved_changes = False - - # # Manually call the change handler, which should add an indication that - # # saves have been done. - # camera_title._title_observer() - # Click save - # assert SaveStatus.SETTING_IS_SAVED in camera_title.title.value - # Now change the camera value, which should trigger the change handler - camera_title._autoui_widget.value = TEST_CAMERA_VALUES - - assert camera_title._autoui_widget.savebuttonbar.unsaved_changes + + # ...and if unsaved_changes is True the "not saved" indicator should be present assert SaveStatus.SETTING_NOT_SAVED in camera_title.title.value + # Click save... + camera_title._autoui_widget.savebuttonbar.bn_save.click() + + # ... cand check that the title is decorated with the "saved" indicator + assert SaveStatus.SETTING_IS_SAVED in camera_title.title.value + # Finally, click the save button and the title should be decorated with # the saved indication. camera_title._autoui_widget.savebuttonbar.bn_save.click() From 09e2fad1ab36b63048b193e3abd9243be89938e8 Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Mon, 15 Jul 2024 08:24:09 -0700 Subject: [PATCH 21/22] Address additional review comments --- stellarphot/gui_tools/comparison_functions.py | 2 ++ stellarphot/settings/custom_widgets.py | 4 +++- stellarphot/settings/tests/test_custom_widgets.py | 14 ++++++++++++-- 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/stellarphot/gui_tools/comparison_functions.py b/stellarphot/gui_tools/comparison_functions.py index cede47d8..4195cfcb 100644 --- a/stellarphot/gui_tools/comparison_functions.py +++ b/stellarphot/gui_tools/comparison_functions.py @@ -261,6 +261,8 @@ def __init__( self.target_coord = object_coordinate self.observatory = observatory + # This function defines several attributes in addition to returning the box and + # image viewer. You should take a look at it to see what it does. self.box, self.iw = self._viewer() self.photometry_settings = PhotometryWorkingDirSettings() diff --git a/stellarphot/settings/custom_widgets.py b/stellarphot/settings/custom_widgets.py index b34e958b..dba4e361 100644 --- a/stellarphot/settings/custom_widgets.py +++ b/stellarphot/settings/custom_widgets.py @@ -816,7 +816,9 @@ def __init__(self, settings, style="tabs", *args, **kwargs) -> None: raise ValueError( f"The {name} setting saved in the working directory is not " f"consistent with the list of {name} items that are saved " - "in your permanent settings. Please fix this manually." + "in your permanent settings. Please fix this manually " + f"by editing your saved {name} settings or by deleting the " + "working directory settings." ) from e # Add symbol to title to indicate that the setting needs review self.badges.append(SaveStatus.SETTING_SHOULD_BE_REVIEWED) diff --git a/stellarphot/settings/tests/test_custom_widgets.py b/stellarphot/settings/tests/test_custom_widgets.py index 6d53cf0d..434f0156 100644 --- a/stellarphot/settings/tests/test_custom_widgets.py +++ b/stellarphot/settings/tests/test_custom_widgets.py @@ -199,14 +199,21 @@ def test_edit_item_not_saved_after_cancel(self): camera = self.make_test_camera() # Make an additional test camera so that the result is the *second* camera - # in the list, not the first. If it is the first the outcome is the same - # whether the user was editing or making a new camera. + # in the list, not the first. The workflow being test is that there are + # already two cameras, then the second one is selected (so that something is) + # selected) and then that second one is edited, but "no" is clicked on the + # confirmation dialog. saved = SavedSettings() camera.name = "zzzz" + camera.name saved.add_item(camera) choose_or_make_new = ChooseOrMakeNew("camera") + + # There should be three choices in the dropdown -- the two cameras and + # "Make new" assert len(choose_or_make_new._choose_existing.options) == 3 + + # Choose the second camera choose_or_make_new._choose_existing.value = camera assert camera == choose_or_make_new.value @@ -990,6 +997,8 @@ def test_creation_no_saved_settings(self, container_type): created_from_defaults = True if created_from_defaults: + # Test whether the setting, which was able to be created from default + # values of the fields, is in the partial_settings. wd_settings.load() snake_name = to_snake(setting_class.__name__) assert ( @@ -1239,6 +1248,7 @@ def test_getting_settings_with_nothing_saved(self): assert review_settings.current_settings == PartialPhotometrySettings() def test_selecting_table_without_saved_setting_sets_proper_badge(self): + # Test that selecting a tab with a saved setting sets a proper "NOT SAVED" badge review_settings = ReviewSettings([PhotometryApertures, Camera]) review_settings._container.selected_index = 1 assert SaveStatus.SETTING_NOT_SAVED in review_settings._container.titles[1] From 4867a3b5dfb3d4138593a167ffd88a7b00f06c52 Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Tue, 16 Jul 2024 11:16:27 -0500 Subject: [PATCH 22/22] Apply suggestions from code review Co-authored-by: Juan Cabanela --- stellarphot/settings/tests/test_custom_widgets.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/stellarphot/settings/tests/test_custom_widgets.py b/stellarphot/settings/tests/test_custom_widgets.py index 434f0156..754614d8 100644 --- a/stellarphot/settings/tests/test_custom_widgets.py +++ b/stellarphot/settings/tests/test_custom_widgets.py @@ -200,7 +200,7 @@ def test_edit_item_not_saved_after_cancel(self): # Make an additional test camera so that the result is the *second* camera # in the list, not the first. The workflow being test is that there are - # already two cameras, then the second one is selected (so that something is) + # already two cameras, then the second one is selected (so that something is # selected) and then that second one is edited, but "no" is clicked on the # confirmation dialog. saved = SavedSettings() @@ -876,7 +876,7 @@ def test_title_decoration_plain_autoui(self): # Click save... camera_title._autoui_widget.savebuttonbar.bn_save.click() - # ... cand check that the title is decorated with the "saved" indicator + # ... and check that the title is decorated with the "saved" indicator assert SaveStatus.SETTING_IS_SAVED in camera_title.title.value # Finally, click the save button and the title should be decorated with