Add a review widget #371

mwcraig · 2024-06-12T18:19:42Z

This pull request adds what will be the first of the new photometry notebooks (setting camera, observatory, passband map) and the final review-of-settings notebook.

Remaining to be done:

add unit tests for ReviewSettings
add notebook for first step
add notebook for last step
add launcher entries for notebooks

Fixes #377 which was uncovered during development of this new widget.
Fixes #182 -- the notebooks were not combined but things have been streamlined a bit.

codecov · 2024-06-12T19:24:59Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.44%. Comparing base (292e181) to head (4867a3b).
Report is 289 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #371      +/-   ##
==========================================
+ Coverage   77.66%   78.44%   +0.78%     
==========================================
  Files          27       27              
  Lines        3626     3758     +132     
==========================================
+ Hits         2816     2948     +132     
  Misses        810      810

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

mwcraig · 2024-06-28T13:36:54Z

Note: the linting failure is because ruff changed its rules -- it is not due to a code change. I'm planning to ignore it for now and open an issue to fix it later.

Handle StrEnum for Python 3.10 Add some explanation to ReviewSettings docstring

The test was not testing the code branch I thought because of a change in the logic in ChooseOrMakeNew.

Rename notebooks to remove spaces

This code is necessary for now because of a bug in ipyautoui, feder-observatory#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.

Remove debugging print

JuanCab

I have reviewed the code, only typos caught and a few clarifications sought. Will test out the notebooks later and report on that.

stellarphot/gui_tools/comparison_functions.py

stellarphot/settings/custom_widgets.py

stellarphot/settings/settings_files.py

stellarphot/settings/tests/test_custom_widgets.py

JuanCab

I have reviewed the code, only typos caught and a few clarifications sought. Will test out the notebooks later and report on that.

JuanCab · 2024-07-14T14:17:58Z

This might be better as a new issue than as something to do for this PR to get merged...

I used the review widget, I noticed that while we got an indication the whole model was bad, there was no way to find out WHY the model was bad. I purposely mis-entered the units for one entry and there was no clear indication that was the problem.

From what I understand of ipyautoui, we are suppressing the actual validation error message (which makes sense). But would it be possible to have a way of displaying the validation error if the user so chooses? Or if you want to get clever you could parse the error message and visually flag the individual bad entries, although that seems like a lot more hard work.

JuanCab · 2024-07-14T14:37:39Z

All the following tests were done in Jupyter Lab, I did test a little with VS Code and it seems consistent except some of the drop downs do NOT render properly. That might be an issue with how iPyAutoui creates its custom widgets...

Other glitches I noticed:

The dropdown menus don't seem to work in expected ways sometimes: Specifically:
- Case 1: Repeated options
  1. Run all cells
  2. Select make a new camera (I used ` from drop down).
  3. Select adu for data units and 1.5 electrons / adu for gain
  4. Glitch? Read noise now presents three identical copies of 10.0 electron for noise.
  5. Glitch? Dark current now presents three identical copies of 0.01 electron / sec for noise.
  6. Glitch? I was presented with the option to select various DN, including two with the wrong, invalid units given all the previous settings. Not necessarily easily avoidable, but I could imagine it as a bit confusing for first-timers at photometry.
- Case 2: No Option
  1. Run all cells
  2. Select to edit the Testing Camera
  3. Go to the dropdown menu for Gain... it has no options. Same with Max Data Value.
Another issue: AAVSO filters appear twice in dropdown
1. I opted to edit the built in Filter wheel 1 under the Passband map tab.
2. I clicked the red + button
3. I added 'r' as a hypothetical filter and selected the dropdown... the R and I filters were listed twice

Co-authored-by: Juan Cabanela <[email protected]>

Tanner728 · 2024-07-15T12:43:20Z

Reviewing this is on my to-do list for tonight. Just FYI @mwcraig

mwcraig · 2024-07-15T22:23:54Z

Case 1: Repeated options ....
Glitch? Read noise now presents three identical copies of 10.0 electron for noise.

Can you double check? The last option is still 10.0 photon for me.

Glitch? Dark current now presents three identical copies of 0.01 electron / sec for noise.

Same here, I'm seeing 0.01 photon / sec as the last option.

Glitch? I was presented with the option to select various DN, including two with the wrong, invalid units given all the previous settings. Not necessarily easily avoidable, but I could imagine it as a bit confusing for first-timers at photometry.

More a design feature, though maybe it ends up being confusing. Each dropdown has three options. The set of first options are consistent with each other, the set of second options are consistent and the set of third options are consistent. However, there is no updating of the suggestions as fields are filled out. That could maybe be added iun the future...

mwcraig · 2024-07-15T22:32:13Z

Case 2: No Option

Run all cells
Select to edit the Testing Camera
Go to the dropdown menu for Gain... it has no options. Same with Max Data Value.

This is because by default ipyautoui makes an ipywidgets.Combobox which is a combination text box and dropdown. The dropdowns are only suggestions; any value is allowed.

In addition, the dropdowns are only shown if there is no text in the box.

You can see the same behavior in the ipywidgets documentation I linked to above.

mwcraig · 2024-07-15T22:41:31Z

Another issue: AAVSO filters appear twice in dropdown
...
I added 'r' as a hypothetical filter and selected the dropdown... the R and I filters were listed twice

Ah, that is because the AAVSO effectively has its own passsband map which maps R and Rc to R and similar for I, so each shows up twice.

Will open a separate issue for this.

mwcraig · 2024-07-15T22:43:03Z

Another issue: AAVSO filters appear twice in dropdown
...
I added 'r' as a hypothetical filter and selected the dropdown... the R and I filters were listed twice

Ah, that is because the AAVSO effectively has its own passsband map which maps R and Rc to R and similar for I, so each shows up twice.

Will open a separate issue for this.

mwcraig · 2024-07-15T22:46:54Z

This might be better as a new issue than as something to do for this PR to get merged...

I used the review widget, I noticed that while we got an indication the whole model was bad, there was no way to find out WHY the model was bad. I purposely mis-entered the units for one entry and there was no clear indication that was the problem.

From what I understand of ipyautoui, we are suppressing the actual validation error message (which makes sense). But would it be possible to have a way of displaying the validation error if the user so chooses?

This is essentially the idea in: maxfordham/ipyautoui#308

For now, can you open an issue in this repo, @JuanCab, to add an option to display the errors, maybe by clicking on the red x?

JuanCab · 2024-07-15T23:35:16Z

Case 1: Repeated options ....
Glitch? Read noise now presents three identical copies of 10.0 electron for noise.

Can you double check? The last option is still 10.0 photon for me.

Glitch? Dark current now presents three identical copies of 0.01 electron / sec for noise.

Same here, I'm seeing 0.01 photon / sec as the last option.

Here are screen shots showing the menus I am seeing:

mwcraig · 2024-07-16T00:05:54Z

Here are screen shots showing the menus I am seeing:

Ooooohhhhh, for a moment I was an old man shaking his fist in the sky in frustration that something was broken, but now I think I know what is going on.

What I said before was not quite right:

In addition, the dropdowns are only shown if there is no text in the [combo] box.

What is displayed in the combobox is only the options consistent with the text entered so far, so when 10.0 electron is already in the read noise, the combobox only shows the two entries consistent with that. If you were to erase the contents of that box you would see three choices, the last of which would be 10.0 photon.

I'm not sure what to do about this....

Tanner728 · 2024-07-16T00:29:16Z

I am starting to review this and just had a quick question. Do we need to provide an input box for the focal length of the system being used? I know we have our FL in the FITS header but does the "Sky" coordinate system, which I assume is WCS, need a focal length to plate solve the images?

mwcraig · 2024-07-16T00:51:30Z

. Do we need to provide an input box for the focal length of the system being used? I know we have our FL in the FITS header but does the "Sky" coordinate system, which I assume is WCS, need a focal length to plate solve the images?

I don't think so -- the FL can be calculated from some of the parameters that the plate solve generates. I think it could also be used as an input, but we haven't done it that way.

I gather, though, that that is something you need to provide as an input for your plate solves?

Tanner728 · 2024-07-16T00:56:04Z

Yeah, from my experience using Pixinsight and ASTAP (standalone platesolver) they need a focal length. It doenst need to be exact but as long as it is within putting distance of the actual FL it works.

mwcraig · 2024-07-16T01:00:13Z

I think we don't need that here because I've been assuming images are already plate solved....

Tanner728

I was not able to break these :(

JuanCab

Looks good, two typo suggestions made, but otherwise, good to go.

stellarphot/settings/tests/test_custom_widgets.py

Co-authored-by: Juan Cabanela <[email protected]>

mwcraig · 2024-07-16T16:17:15Z

Looks good, two typo suggestions made, but otherwise, good to go.

Merging once tests pass 😬 🎉

mwcraig added this to the 2.0.0 -- Reimplemented data structure milestone Jun 12, 2024

mwcraig marked this pull request as draft June 12, 2024 18:19

mwcraig mentioned this pull request Jun 13, 2024

Use pytest-mock to create testing path for saved settings #372

Merged

mwcraig force-pushed the first-step-notebook branch 2 times, most recently from 78a663c to 0925b09 Compare June 13, 2024 18:22

mwcraig changed the title ~~Refactor title decoration and review widget~~ Add a review widget Jun 13, 2024

mwcraig force-pushed the first-step-notebook branch 3 times, most recently from b29a412 to bb66aba Compare June 20, 2024 23:20

mwcraig force-pushed the first-step-notebook branch 4 times, most recently from 5ac48ec to d427bfd Compare July 1, 2024 21:51

mwcraig added 15 commits July 2, 2024 08:16

Draft of tests for ReviewSettings

3f86e09

Refactor title decoration and add draft review widget

e77feeb

Handle StrEnum for Python 3.10 Add some explanation to ReviewSettings docstring

Refactor ChooseOrMakeNew and badges

3a154f8

Update tests to match updated classes

4061ae3

Improve test coverage in ReviewSettings

55bf455

Fix logic in handling confirmation logic and test

5a377b9

The test was not testing the code branch I thought because of a change in the logic in ChooseOrMakeNew.

Fix ruff command in tox.ini

b949e09

Test for and fix a case where badges were not set properly

6f66ad5

Allow full settings to be updated from partial

6cb0b9a

Fix saving and tab status for settings with default values

a07792e

Initial draft of review notebooks

3e78c6c

Rename notebooks to remove spaces

Update logic when tab is selected

18bb921

Remove debugging print

Use a temporary settings directory in tests

e716fa7

Add new notebooks to jupyterlab launcher

88a8668

JuanCab approved these changes Jul 12, 2024

View reviewed changes

JuanCab previously approved these changes Jul 12, 2024

View reviewed changes

Apply suggestions from code review by @JuanCab

d350c98

Co-authored-by: Juan Cabanela <[email protected]>

mwcraig dismissed JuanCab’s stale review via d350c98 July 15, 2024 00:50

Clarify intent of test steps and remove cruft

0b20545

Address additional review comments

09e2fad

mwcraig mentioned this pull request Jul 15, 2024

Repeated entries in AAVSO filters for PassbandMap #382

Closed

mwcraig requested a review from JuanCab July 15, 2024 22:47

JuanCab mentioned this pull request Jul 15, 2024

Few hints as to what is making the pydantic model invalid #383

Open

Tanner728 approved these changes Jul 16, 2024

View reviewed changes

JuanCab previously approved these changes Jul 16, 2024

View reviewed changes

stellarphot/settings/tests/test_custom_widgets.py Outdated Show resolved Hide resolved

stellarphot/settings/tests/test_custom_widgets.py Outdated Show resolved Hide resolved

mwcraig dismissed JuanCab’s stale review via 4867a3b July 16, 2024 16:16

Apply suggestions from code review

4867a3b

Co-authored-by: Juan Cabanela <[email protected]>

mwcraig merged commit 2e0f41b into feder-observatory:main Jul 16, 2024
12 checks passed

mwcraig deleted the first-step-notebook branch July 16, 2024 19:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a review widget #371

Add a review widget #371

mwcraig commented Jun 12, 2024 •

edited

Loading

codecov bot commented Jun 12, 2024 •

edited

Loading

mwcraig commented Jun 28, 2024

JuanCab left a comment

JuanCab left a comment

JuanCab commented Jul 14, 2024

JuanCab commented Jul 14, 2024 •

edited

Loading

Tanner728 commented Jul 15, 2024

mwcraig commented Jul 15, 2024

mwcraig commented Jul 15, 2024

mwcraig commented Jul 15, 2024

mwcraig commented Jul 15, 2024

mwcraig commented Jul 15, 2024

JuanCab commented Jul 15, 2024 •

edited

Loading

mwcraig commented Jul 16, 2024

Tanner728 commented Jul 16, 2024

mwcraig commented Jul 16, 2024

Tanner728 commented Jul 16, 2024

mwcraig commented Jul 16, 2024

Tanner728 left a comment

JuanCab left a comment

mwcraig commented Jul 16, 2024

Add a review widget #371

Add a review widget #371

Conversation

mwcraig commented Jun 12, 2024 • edited Loading

codecov bot commented Jun 12, 2024 • edited Loading

Codecov Report

mwcraig commented Jun 28, 2024

JuanCab left a comment

Choose a reason for hiding this comment

JuanCab left a comment

Choose a reason for hiding this comment

JuanCab commented Jul 14, 2024

JuanCab commented Jul 14, 2024 • edited Loading

Tanner728 commented Jul 15, 2024

mwcraig commented Jul 15, 2024

mwcraig commented Jul 15, 2024

mwcraig commented Jul 15, 2024

mwcraig commented Jul 15, 2024

mwcraig commented Jul 15, 2024

JuanCab commented Jul 15, 2024 • edited Loading

mwcraig commented Jul 16, 2024

Tanner728 commented Jul 16, 2024

mwcraig commented Jul 16, 2024

Tanner728 commented Jul 16, 2024

mwcraig commented Jul 16, 2024

Tanner728 left a comment

Choose a reason for hiding this comment

JuanCab left a comment

Choose a reason for hiding this comment

mwcraig commented Jul 16, 2024

mwcraig commented Jun 12, 2024 •

edited

Loading

codecov bot commented Jun 12, 2024 •

edited

Loading

JuanCab commented Jul 14, 2024 •

edited

Loading

JuanCab commented Jul 15, 2024 •

edited

Loading