UI work #278

mwcraig · 2024-02-23T17:13:26Z

@JuanCab -- if this passes tests I'm going to merge it so we can have a couple folks try out some of the settings forms.

mwcraig · 2024-02-23T20:53:13Z

Actually, just kidding about the merging, there is a better way to handle it. In any event, another bug needs to get fixed first.

This makes it sort of act like a dict even though it is not.

mwcraig · 2024-02-26T19:36:52Z

@JuanCab -- the thing to focus on here is the new PassbandMap model. The old one (a stright-up dict) didn't have an automatically generate-able UI. The new one does. On the other hand, the new is a PITA to work with, so I made it settable from a dict, and added just enough methods to make it behave like a dict. Not at all sure this is the best way to go, but we can always change it later I suppose.

I'll get folks looking at the UI separately from getting this merged.

JuanCab

I think I followed this in the end, but would appreciate answers to some of my questions. You need to fix all the mentions of passband_map in the docstrings to indicate it is now a PassbandMap object! Otherwise, while ugly, if it passes the tests, then we can say it works well enough.

JuanCab · 2024-02-27T03:02:43Z

autoui_widget_viewer.ipynb

Did note review notebook beyond GitHub changes interface, looks like mostly markdown changes anyway.

JuanCab · 2024-02-27T03:04:08Z

docs/index.rst

+Creating an environment with `conda` or `mamba`:
+
+```bash
+mamba create -n stellarphot-test python=3.11


So does stellarphot-test have all the dependencies defined? I have my own testing environment for stellarphot under conda and I should probably compare the two setups.

No -- this section should be modified/removed. For testing of this branch I sent the instructions below. It creates a bare-bones environment then pip installs everything from a github URL so people can try out the UI independent of whether we've done a pre-release or not and dodging all of the intricacies of git:

This first look will be to try out a couple of jupyter notebook-based forms for entering a bunch of settings.

I’d recommend, for now, that when you are testing you make a python environment for testing in, the delete the environment when you are done instead of trying to frequently update stellarphot. Instructions for doing that if you use conda are below, happy to provide non-conda instructions too.

To make an environment for testing called sp-test:

conda create -n sp-test python=3.11

When the environment is created, activate it:

conda ativaste sp-test

The command to install the version of stellarphot to try this time around is:

pip install git+https://github.com/mwcraig/stellarphot.git@ui-work

When this is done, please save the attached notebook to a folder, and navigate into that folder in the terminal. Run “jupyter lab” to start up jupyter and try going through the notebook. After several cells of set up it will present forms for you to enter settings. Don’t worry about using realistic values for now.

I’m expecting you will have questions and that it needs more instructions, but figured getting a quick look sooner was better than waiting longer.

When you are done you can deactivate the environment and then remove it:

conda deactivate conda remove --all --yes sp-test

JuanCab · 2024-02-27T03:04:50Z

stellarphot/core.py

-            self._passband_map[orig_pb] if orig_pb in self._passband_map else orig_pb
+            (
+                self._passband_map[orig_pb]
+                if orig_pb in self._passband_map.keys()


Isn't the default iterator for a dictionary keys? Oh, I see, you defined a method keys for the PassbandModel that returns the keys. Later I do see where you perform self._passband_map = passband_map.model_copy() to set self._passband_map... but passband_map is a dict according to the docstrings forCatalogData and PhotometryData . Does this need to get changed to PassbandMap object?

Did you place this function the BaseEnhancedTable so it is available to be inherited by CatalogData and PhotometryData? I ask because the passband mapping is not used in the BaseEnhancedTable.

Finally, what is self["passband"]? I can't find where self["passband"] is even defined in BaseEnhancedTable?

Taking a look in a moment -- the intent was to have every place use a PassbandMap object but to make that object act sort of like a dict.

Also, you are write that the default iterator is over keys, but I wasn't sure which dunder method I needed to use to make that happen. Can dig it up in Fluent Python probably.

Did you place this function the BaseEnhancedTable so it is available to be inherited by CatalogData and PhotometryData? I ask because the passband mapping is not used in the BaseEnhancedTable.

I think _update_passbands has been in BaseEnhancedTable for a while: https://github.com/feder-observatory/stellarphot/blob/main/stellarphot/core.py#L166

I do think you are correct about the reason it is in BaseEnhancedTable

Finally, what is self["passband"]?

Took me a sec to remember, it is one of the columns in the table.

JuanCab · 2024-02-27T03:19:49Z

stellarphot/settings/models.py

@@ -545,27 +564,81 @@ class PhotometryOptions(BaseModelWithTableRep):
    fwhm_by_fit: bool = True


+class PassbandMapEntry(BaseModel):
+    """
+    long loing liong liong liong long long


What is this?

oops, meant to edit that 😬 it was keyboard mashing.

stellarphot/core.py

JuanCab · 2024-02-27T03:26:23Z

stellarphot/photometry/tests/test_photometry.py

-    "B": "B",
-    "rp": "SR",
-}
+PASSBAND_MAP = PassbandMap(


So this is using the pydantic model... but stellarphot/core wasn't?

JuanCab · 2024-02-27T03:27:47Z

stellarphot/settings/models.py

-        A dictionary containing instrumental passband names as keys and
-        AAVSO passband names as values. This is used to rename the passband
-        entries in the output photometry table.
+    your_filter_names_to_aavso : list[`stellarphot.settings.PassbandMapEntry`]


OK, this makes sense, although again, not seeing how stellarphot/core.py knows about this?

stellarphot/settings/models.py

This also tests them and improves the documentation

mwcraig · 2024-02-27T17:47:32Z

@JuanCab -- I think this is ready for another look

codecov-commenter · 2024-02-27T17:53:03Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.29%. Comparing base (85c37b1) to head (4dabd8d).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #278      +/-   ##
==========================================
+ Coverage   70.09%   71.29%   +1.19%     
==========================================
  Files          24       25       +1     
  Lines        3023     3100      +77     
==========================================
+ Hits         2119     2210      +91     
+ Misses        904      890      -14

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

JuanCab

My only issue is in the docs of stellarphot/core.py you refer to PassbandMap as a dictionary. I do think you have a point in that it certainly behaves like a dictionary, BUT it isn't created like dict... don't know if that will confuse people who actually read the docstrings but are not familiar with pydantic.

JuanCab · 2024-02-29T01:21:59Z

stellarphot/core.py

@@ -284,7 +285,7 @@ class PhotometryData(BaseEnhancedTable):
        names as values.  This is used to automatically update the column
        names to the desired names before the validation is performed.

-    passband_map: dict, optional (Default: None)
+    passband_map: `stellarphot.settings.PassbandMap`, optional (Default: None)
        A dictionary containing instrumental passband names as keys and


Do you want to call it a dictionary in the documentation? I guess It is a dictionary behaviorally, but it is not a dict. For one thing, creating a PassbandMap object requires a very different syntax.

What happens if the user reads the docstring and just passes in a dict? It should fail (or convert?). Maybe we add an Exception that reports they passed in a dict and tells them how to convert it into a proper PassbandMap object?

Oops, that was completely an oversight on my part. I'll change the wording here and below.

JuanCab · 2024-02-29T01:22:37Z

stellarphot/core.py

@@ -603,7 +604,7 @@ class CatalogData(BaseEnhancedTable):
        names as values.  This is used to automatically update the column
        names to the desired names BEFORE the validation is performed.

-    passband_map: dict, optional (Default: None)
+    passband_map: `stellarphot.settings.PassbandMap`, optional (Default: None)
        A dictionary containing instrumental passband names as keys and


Same quibble with calling it a dictionary as above.

JuanCab · 2024-02-29T01:22:57Z

stellarphot/io/tests/test_tess_submission.py

@@ -88,6 +88,10 @@ def test_target_file():
    except ConnectionError:
        server_down = True
        tess_target = None  # Assure tess_target is defined so that we can delete it
+    except ValueError:
+        # The server is teechnically back but producing garbage....


"technically" not "teechnically"...

JuanCab · 2024-02-29T01:23:42Z

stellarphot/settings/models.py

    use_coordinates: Literal["sky", "pixel"] = "sky"
    shift_tolerance: NonNegativeFloat = 5.0


-class PhotometryOptions(BaseModelWithTableRep):
+class PhotometryOptionalSettings(BaseModelWithTableRep):


I agree, better name.

JuanCab · 2024-02-29T01:26:11Z

stellarphot/settings/models.py

I like how you added support to make PassbandMap behave like a dict in many ways.

That was partly bcc it was easier to do that than it was to rewrite code that used PassbandMap 🤣

mwcraig · 2024-02-29T15:32:31Z

@JuanCab -- made the changes you suggested. If test pass I think this can be merged/

JuanCab

OK, looks good.

mwcraig added this to the 2.0.0 -- Reimplemented data structure milestone Feb 23, 2024

mwcraig added 9 commits February 25, 2024 16:47

Shorten default descriptions for models

6d78bf0

Add models for passband entry

a48104d

Add widget preview notebook for easier review

9d6c3fc

Add some default settings to auto-generated ui

cff0596

Add title/description to PhotometrySetting ui

355bdb0

Update auto ui notebook

cba75be

Add installation instructinos

021784a

Make source location have a better ui

be4608e

Get widget viewer ready for testing

fe3443b

mwcraig force-pushed the ui-work branch from 3bbabe9 to fe3443b Compare February 25, 2024 22:47

mwcraig added 2 commits February 26, 2024 13:01

Rewrite PassbandMap model

103ec03

This makes it sort of act like a dict even though it is not.

Add more detail to settings review notebook

ee44133

mwcraig requested a review from JuanCab February 26, 2024 19:29

JuanCab reviewed Feb 27, 2024

View reviewed changes

mwcraig added 4 commits February 27, 2024 11:42

Add more dictionary methods to PassbandMap

1eaf766

This also tests them and improves the documentation

Update docs in core to reflect new passband map

6f6c44e

Add another failure route for TESS target file test

06b46c4

Remove debugging statement

e8d3e07

mwcraig requested a review from JuanCab February 27, 2024 17:47

mwcraig added 3 commits February 27, 2024 12:33

Add test coverage for missing lines and fix bug

77d2fd2

Change PhotometryOptions to PhotometryOptionalSettings

224af69

Remove workarounds for issue fixed in ipyautoui

10dbe94

JuanCab reviewed Feb 29, 2024

View reviewed changes

Address review comments

4dabd8d

JuanCab self-requested a review February 29, 2024 17:01

JuanCab approved these changes Feb 29, 2024

View reviewed changes

mwcraig merged commit 43b121f into feder-observatory:main Feb 29, 2024
9 checks passed

mwcraig deleted the ui-work branch April 29, 2024 15:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI work #278

UI work #278

mwcraig commented Feb 23, 2024

mwcraig commented Feb 23, 2024

mwcraig commented Feb 26, 2024

JuanCab left a comment

JuanCab Feb 27, 2024

JuanCab Feb 27, 2024

mwcraig Feb 27, 2024

JuanCab Feb 27, 2024

mwcraig Feb 27, 2024

mwcraig Feb 27, 2024

mwcraig Feb 27, 2024

mwcraig Feb 27, 2024

JuanCab Feb 27, 2024

mwcraig Feb 27, 2024

JuanCab Feb 27, 2024

JuanCab Feb 27, 2024

mwcraig commented Feb 27, 2024

codecov-commenter commented Feb 27, 2024 •

edited

Loading

JuanCab left a comment

JuanCab Feb 29, 2024 •

edited

Loading

mwcraig Feb 29, 2024

JuanCab Feb 29, 2024

JuanCab Feb 29, 2024

JuanCab Feb 29, 2024

JuanCab Feb 29, 2024

mwcraig Feb 29, 2024

mwcraig commented Feb 29, 2024

JuanCab left a comment

UI work #278

UI work #278

Conversation

mwcraig commented Feb 23, 2024

mwcraig commented Feb 23, 2024

mwcraig commented Feb 26, 2024

JuanCab left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwcraig commented Feb 27, 2024

codecov-commenter commented Feb 27, 2024 • edited Loading

Codecov Report

JuanCab left a comment

Choose a reason for hiding this comment

JuanCab Feb 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwcraig commented Feb 29, 2024

JuanCab left a comment

Choose a reason for hiding this comment

codecov-commenter commented Feb 27, 2024 •

edited

Loading

JuanCab Feb 29, 2024 •

edited

Loading