Autostronomy / AstroPhot

A fast, flexible, automated, and differentiable astronomical image 2D forward modelling tool for precise parallel multi-wavelength photometry
https://astrophot.readthedocs.io
GNU General Public License v3.0
87 stars 9 forks source link

more helpful str and repr for users #129

Closed ConnorStoneAstro closed 11 months ago

ConnorStoneAstro commented 11 months ago

closes #128 by making str and repr print out useful parameter values in a clear format. Example str for sersic model:

model1:
center: [50.0, 50.0] +- 0.1 [arcsec]
q: 0.6 +- 0.03 [b/a], limits: (0.0, 1.0)
PA: 1.0471975511965976 +- 0.06 [radians], limits: (0.0, 3.141592653589793), cyclic
n: 2.0 +- 0.05 [none], limits: (0.36, 8.0)
Re: 10.0 [arcsec], limits: (0.0, None)
Ie: 1.0 [log10(flux/arcsec^2)]

And example repr:

model1 (id-140248206704112, branch node):
  center (id-140248206702048): [50.0, 50.0] +- 0.1 [arcsec]
  q (id-140248206702000): 0.6 +- 0.03 [b/a], limits: (0.0, 1.0)
  PA (id-140248206701952): 1.0471975511965976 +- 0.06 [radians], limits: (0.0, 3.141592653589793), cyclic
  n (id-140248206701904): 2.0 +- 0.05 [none], limits: (0.36, 8.0)
  Re (id-140248206701856): 10.0 [arcsec], limits: (0.0, None)
  Ie (id-140248206701808): 1.0 [log10(flux/arcsec^2)]

Note the indenting for the repr, this continues for each level of the graph and it useful for complex parameter DAGs

codecov[bot] commented 11 months ago

Codecov Report

Merging #129 (14550ba) into main (fd815e3) will increase coverage by 0.05%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #129      +/-   ##
==========================================
+ Coverage   77.83%   77.88%   +0.05%     
==========================================
  Files          80       80              
  Lines        6951     6967      +16     
==========================================
+ Hits         5410     5426      +16     
  Misses       1541     1541              
Files Coverage Δ
astrophot/models/core_model.py 76.65% <100.00%> (+0.20%) :arrow_up:
astrophot/param/base.py 95.74% <100.00%> (ø)
astrophot/param/parameter.py 94.72% <100.00%> (+0.22%) :arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

wmwv commented 11 months ago

Here's a short test I wrote for tests/test_model.TestAllModelBasics that triggers the infinite recursion error. It fails on current main and passes on this printparams branch.

    def test_initialize_does_not_recurse(self):
        "Test case for error where missing parameter name triggered print that triggered missing parameter name ..."
        target = make_basic_sersic()
        model = ap.models.AstroPhot_Model(
            name="test model",
            model_type="sersic galaxy model",
            target=target,
        )
        # Define a function that accesses a parameter that doesn't exist
        def calc(params):
            return params["A"].value

        model["center"].value = calc

        with self.assertRaises(ValueError) as context:
            model.initialize()
        self.assertTrue(str(context.exception) == "Unrecognized key for 'center': A")
ConnorStoneAstro commented 11 months ago

Here's a short test I wrote for tests/test_model.TestAllModelBasics that triggers the infinite recursion error. It fails on current main and passes on this printparams branch.

Awesome, thanks! I've been working on an explicit print test for str and repr. I'll add in this test to the PR and it should be good to go! Probably sometime this afternoon.

wmwv commented 11 months ago

Hmmm... I see, the repr seems a bit tricky because target_identity is not deterministic.

The str tests seems simpler:

expected_model_str = "test model:\ncenter: [19.737866914381733, 19.60972003061572] +- 0.1 [arcsec]\nPA: 2.4637503765878934 +- 0.024637503765878935 [rad], limits: (0.0, 3.141592653589793), cyclic\nI0: -0.7234624674314032 +- 0.28508068034870027 [log10(flux/arcsec^2)]\nhs: 4.0 +- 2.0 [arcsec], limits: (0.0, None)\nrs: 16.0 +- 8.0 [arcsec], limits: (0.0, None)"
self.assertTrue(str(MODEL) == expected_model_str)
ConnorStoneAstro commented 11 months ago

just committed the tests I was working on. It removes the ids for the repr