Update Camera object #231

mwcraig · 2023-12-22T17:20:33Z

This ended up doing too many things -- please let me know if you want me to break it into smaller PRs.

Adds "max_adu" as a Camera attribute. I named it max_data_val instead, because...
...allow any data unit, but require user to say what it is by adding a data_unit attribute to Camera.
Fix Modify Camera model so that it can generate a JSON schema so that we can generate a UI #165 by indicating how schema should be created for the types we define.
Adds a bit of description to each field and some examples, which end up being displayed in the auto-generated UI.
Drops the copy method because pydantic provides one
Drops the __repr__ because pydantic provides one

(and did 5 and 6 to avoid having to update them every time a change is made)

This fixes feder-observatory#165, which allows a UI to be autogenerated.

codecov-commenter · 2023-12-22T17:26:30Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d35a332) 53.80% compared to head (cd69d8c) 54.44%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #231      +/-   ##
==========================================
+ Coverage   53.80%   54.44%   +0.64%     
==========================================
  Files          23       23              
  Lines        2881     2911      +30     
==========================================
+ Hits         1550     1585      +35     
+ Misses       1331     1326       -5

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

JuanCab

As far as I can tell, it looks good. I am not that experienced with pydantic, but otherwise, everything seemed reasonable. I'll approve it.

JuanCab · 2023-12-22T21:04:02Z

stellarphot/core.py

@@ -30,6 +30,36 @@
 # https://docs.gammapy.org/dev/_modules/gammapy/analysis/config.html


+class UnitType(Unit):


So I assume this is adding pydantic validators to the astropy units class? Do you know if it is pydantic v2 compatible? There is a part of me that worries that pydantic v2 is still waiting to bite us in the future.

JuanCab · 2023-12-22T21:09:46Z

stellarphot/core.py

+        yield cls.validate
+
+    @classmethod
+    def validate(cls, v):


What is this validate doing? It just returns the Unit cast version of v? Does it actually validate anything?

If the creation of the unit fails (e.g. Unit("nonsense")) then it generates a ValidationError

JuanCab · 2023-12-22T21:10:28Z

stellarphot/core.py

+
+    @classmethod
+    def __modify_schema__(cls, field_schema, field):
+        # Set default values for the schema in case the field doesn't provide them


I am honestly not sure how to assess what is going on here with __modify_schema__, need to dig into pydantic again or trust your tests. :). I will note __modify_schema__ doesn't appear to be supported in v2 of pydantic (based on https://docs.pydantic.dev/latest/errors/usage_errors/#custom-json-schema).

To the extent I understand it, this is providing some of the information needed to generate a json schema for types that are either not built in to Python or pydantic.

I wish ipyautoui had already made the 1 to 2 transition so we could just code for 2. It looks like there is a replacement for __modify_schema__ in 2.

JuanCab · 2023-12-22T21:12:07Z

stellarphot/core.py

+        name = "Unit"
+        description = "An astropy unit"
+
+        name = field.name or name


Is this setting name to a true/false value based on field.name or name as a boolean?

This sets name to field.name unless field.name evaluates to False, in which case name is used.

JuanCab · 2023-12-22T21:15:46Z

stellarphot/core.py

+        examples=["10.0 electron"],
+    )
+    dark_current: QuantityType = Field(
+        description="unit consisten with read noise, per unit time",


"unit consistent" (typo)

JuanCab · 2023-12-22T21:22:47Z

stellarphot/photometry/tests/test_photometry.py

@@ -22,10 +22,25 @@
 )
 from stellarphot.settings import ApertureSettings

+# Constants for the tests
+
 GAINS = [1.0, 1.5, 2.0]
 # Make sure the tests are deterministic by using a random seed


Really not 'using a random seed', rather you are forcing a 'fixed seed for random number generator'

That is true...

JuanCab · 2023-12-22T21:23:55Z

stellarphot/photometry/tests/test_photometry.py

 GAINS = [1.0, 1.5, 2.0]
 # Make sure the tests are deterministic by using a random seed
 SEED = 5432985

+SHIFT_TOLERANCE = 6


Good call to move these toward the front of the testing code.

JuanCab · 2023-12-22T21:25:07Z

stellarphot/photometry/tests/test_photometry.py

Looks good, seems a pretty straight forward set of changes to using proper capitalized constants and to removing max_adu as an argument.

JuanCab · 2023-12-22T21:26:39Z

stellarphot/tests/test_core.py

-        c = Camera(
+    max_adu = 50000 * u.adu
+
+    # All 5 of the attributes after data_unit will be checked for units


Cool, I didn't know you could do this with pytest, counting the number of errors.

It isn't counting the errors, pydantic is -- pytest is just checking that the text of the error message contains "5 validation errors"

JuanCab · 2023-12-22T21:28:15Z

stellarphot/tests/test_core.py

+
+
+def test_camera_schema():
+    # Check that we can generate a schema for a Camera and that it


I assume this schema method is a feature of pydantic...

Yeah; it is used by ipyautoui when generating the UI.

mwcraig added 4 commits December 21, 2023 11:15

Fix up the imports

707ff71

Add max_adu to Camera as max_data_value

e5271e6

Add data_unit to camera and update tests

2735f56

Add schema to type declarations

dd0e3cc

This fixes feder-observatory#165, which allows a UI to be autogenerated.

mwcraig added this to the 2.0.0 -- Reimplemented data structure milestone Dec 22, 2023

mwcraig requested a review from JuanCab December 22, 2023 17:20

Fix notebook construction of Camera

5eec456

mwcraig added 4 commits December 22, 2023 12:06

Remove print functions

890883a

Add test of Camera schema generation

d72f320

Test some corner cases to improve coverage

c0a4d55

Fix argument type in docstring

e03eb8f

JuanCab previously approved these changes Dec 22, 2023

View reviewed changes

Address review comments

cd69d8c

mwcraig dismissed JuanCab’s stale review via cd69d8c December 23, 2023 17:27

mwcraig merged commit 13eb762 into feder-observatory:main Dec 24, 2023
9 checks passed

mwcraig mentioned this pull request Dec 31, 2023

Check camera json encoding #237

Merged

mwcraig deleted the photometry-notebook-settings branch February 8, 2024 15:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Camera object #231

Update Camera object #231

mwcraig commented Dec 22, 2023 •

edited

Loading

codecov-commenter commented Dec 22, 2023 •

edited

Loading

JuanCab left a comment

JuanCab Dec 22, 2023

JuanCab Dec 22, 2023

mwcraig Dec 22, 2023

JuanCab Dec 22, 2023

mwcraig Dec 22, 2023

JuanCab Dec 22, 2023

mwcraig Dec 22, 2023

JuanCab Dec 22, 2023

JuanCab Dec 22, 2023

mwcraig Dec 22, 2023

JuanCab Dec 22, 2023

JuanCab Dec 22, 2023

JuanCab Dec 22, 2023

mwcraig Dec 22, 2023

JuanCab Dec 22, 2023

mwcraig Dec 22, 2023

		@@ -30,6 +30,36 @@
		# https://docs.gammapy.org/dev/_modules/gammapy/analysis/config.html


		class UnitType(Unit):



		def test_camera_schema():
		# Check that we can generate a schema for a Camera and that it

Update Camera object #231

Update Camera object #231

Conversation

mwcraig commented Dec 22, 2023 • edited Loading

codecov-commenter commented Dec 22, 2023 • edited Loading

Codecov Report

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

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 Dec 22, 2023 •

edited

Loading

codecov-commenter commented Dec 22, 2023 •

edited

Loading