Add Exoplanet Object #235

Tanner728 · 2023-12-26T16:00:05Z

Created the exoplanet object and added documentation for it based on the documentation of the aperture settings class above

Fixes #186

codecov-commenter · 2023-12-26T16:04:48Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d35a332) 53.80% compared to head (2d5c764) 55.17%.
Report is 24 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #235      +/-   ##
==========================================
+ Coverage   53.80%   55.17%   +1.37%     
==========================================
  Files          23       23              
  Lines        2881     2954      +73     
==========================================
+ Hits         1550     1630      +80     
+ Misses       1331     1324       -7

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

mwcraig

Excellent! I left a number o minor inline comments below.

In addition, can you please add a test to check that you get a validation error when you pass in a period and a duration without a unit (or with the wrong kind of unit)?

stellarphot/settings/models.py

mwcraig · 2023-12-26T20:47:26Z

stellarphot/settings/models.py

+    To create an `Exoplanet` object, you can pass in the epoch,
+    period, Identifier, coordinate, depth, and duration as keyword arguments:
+
+    >>> planet  = Exoplanet(epoch=Time(0, format="jd"), period=0 * u.min,


It is probably more helpful if you put in information for a real exoplanet. Maybe Kelt-1?

updated to use kelt-1b info but all of it may not be correct. kinda estimated values based on the graphs from a paper i found. also the depth is based on normalized flux

You can look up exoplanet data here: https://exoplanetarchive.ipac.caltech.edu/

There are likely several values for kelt-1, maybe use the most recent.

stellarphot/settings/tests/test_models.py

mwcraig · 2023-12-27T16:23:57Z

stellarphot/settings/models.py

+        Depth of the exoplanet.
+
+    duration : `astropy.units.Quantity`, optional
+        Duration of the exoplanet transit as a Quantity with units of time,


Please update this description

mwcraig · 2023-12-27T16:24:36Z

stellarphot/settings/tests/test_models.py

 from pydantic import ValidationError
 import pytest

-from stellarphot.settings.models import ApertureSettings
+from stellarphot.settings.models import ApertureSettings,Exoplanet


Suggested change

from stellarphot.settings.models import ApertureSettings,Exoplanet

from stellarphot.settings.models import ApertureSettings, Exoplanet

mwcraig · 2023-12-27T16:25:19Z

Looking good -- the main thing still missing is a test for invalid values for period and duration.

Tanner728 · 2023-12-27T16:27:45Z

For the validation test would that be similar to this:

@pytest.mark.parametrize("bad_one", ["radius", "gap", "annulus_width"])
def test_create_invalid_values(bad_one):
    # Check that individual values that are bad raise an error
    bad_settings = DEFAULT_APERTURE_SETTINGS.copy()
    bad_settings[bad_one] = -1
    with pytest.raises(ValidationError, match=bad_one):
        ApertureSettings(**bad_settings)

mwcraig · 2023-12-27T16:30:58Z

Yep, something like that would work

mwcraig · 2023-12-27T17:44:01Z

stellarphot/settings/tests/test_models.py

+    assert u.get_physical_type(planet.duration) == "time"
+
+
+@pytest.mark.parametrize("bad_one", [


Usually parametrize is used to when you want to test with multiple values rather than a single value. I could see going two ways here:

Test two values, like in the example you mentioned here. That maybe looks like @pytest.mark.parametrize('bad_one', ('period', 'duration')) and then in the test set the value ot something bad like in the example.

Test with one set of settings in which both duration and period are bad (inappropriate units) and make sure the error message has the right number of ValidationErrors. See this test for an example of that approach.

Either way, you should be able to start with a copy of the default exoplanet and modify values of that rather than reproducing all the settings.

So I redid the tests and just have a question about the order of operation for the tests. Does this pytest run the validation tests that are in the exoplanet class that check the physical units of duration and period? When I gave period and duration units of cm it would pass this pytest.

It should run the validation checks and then raise a single ValidationError (rather than several ValueErrors) with a summary of the errors it encountered.

mwcraig · 2023-12-27T17:45:29Z

stellarphot/settings/tests/test_models.py

+])
+def test_create_invalid_exoplanet(bad_one):
+    # Check that individual values that are bad raise an error
+    with pytest.raises(ValidationError, match=".*"):


Typically if match is included you should try to match some meaningful part of the error message. This particular value ".*" means it will always match, which is equivalent to not having match there at all.

mwcraig · 2023-12-27T17:48:00Z

A couple broader comments:

Please try to fix the linting issues before committing so there aren't a bunch of lint fix commit.
Don't rely on what VS Code says is too long a line unless you update the line length to 88. It seems like in several cases you have "overshortened" the lines.

Tanner728 · 2023-12-27T18:40:49Z

should i just ignore the 'line too long' issues or is there something else I can check to see if lines are too long?

mwcraig · 2023-12-27T20:23:46Z

should i just ignore the 'line too long' issues or is there something else I can check to see if lines are too long?

I would run tox -e lint before you commit -- if it doesn't complain about lines being too long then you are OK. When we talked yesterday I thought your vscode was marking lines longer than 79 characters even though the project line length limit is 88.

mwcraig · 2023-12-27T21:01:04Z

stellarphot/settings/models.py

+            Time: lambda v: f"{v.value}",
+        }
+
+    @classmethod


this needs one more decorator to finish turning it into something that pydantic actually validates, something like @validator("period") (after importing validator from pydantic).

See https://github.com/feder-observatory/stellarphot/blob/main/stellarphot/core.py#L289 for an example. That is why leaving the units off didn't generate a validation error. Let's ,make sure it does by only making those two values bad.

@Tanner728 -- do you have time to add this change or should I wrap it up instead?

I should have time tonight to do it.

Looks like you left out the "period" argument to @validator....

Tanner728 · 2024-01-01T20:13:14Z

After adding the validator I get this error message when running the tests and am somewhat stumped about where its coming from.

py::test_create_exoplanet_correctly failed: def test_create_exoplanet_correctly():

  planet = Exoplanet(**DEFAULT_EXOPLANET_SETTINGS)

stellarphot\settings\tests\test_models.py:53:

???
E pydantic.error_wrappers.ValidationError: 1 validation error for Exoplanet
E period
E 'Quantity' object with a scalar value does not support indexing (type=type_error)

pydantic\main.py:341: ValidationError

mwcraig · 2024-01-01T21:47:28Z

Can you push the commit the change you made or paste the updarted code into a comment so I can take a look?

mwcraig · 2024-01-01T22:20:35Z

Looks like you pushed a merge commit? You shouldn't need a merge commit....

mwcraig · 2024-01-02T01:04:27Z

Looks like you left out the "period" argument to @validator...

(cherry picked from commit d9736f3) (cherry picked from commit 3d7617e)

(cherry picked from commit db220b3) fixing linting issues Linting fixes

Added the '@validator' decorator to TimeType, runs into QuantityType error on the period attribute

mwcraig · 2024-01-02T18:42:33Z

FYI, just updated this to finish off the validators (I think I did, at least, will see what the tests say)

mwcraig

Thanks, @Tanner728

Tanner728 · 2024-01-02T19:40:51Z

Thank you @mwcraig. I am currently driving out to Montana but will send you a draft of the proceedings paper tonight. Thanks for finishing that validator off.

mwcraig requested changes Dec 26, 2023

View reviewed changes

mwcraig reviewed Dec 27, 2023

View reviewed changes

Tanner728 and others added 8 commits January 2, 2024 12:36

Add Exoplanet model

d08298a

(cherry picked from commit d9736f3) (cherry picked from commit 3d7617e)

Update formatting

b99c0aa

(cherry picked from commit db220b3) fixing linting issues Linting fixes

Add docum to validate functions

f5d1d1b

Update Documentation

87878ba

add validation tests

b4bc93b

fix linting errors

9766bff

adjusted unit testing

9516146

Add validator decorator

417e0ba

Added the '@validator' decorator to TimeType, runs into QuantityType error on the period attribute

mwcraig force-pushed the exoplanet branch from a0f12d1 to f04a6f8 Compare January 2, 2024 18:41

Finish validators in exoplanet model

2d5c764

mwcraig force-pushed the exoplanet branch from f04a6f8 to 2d5c764 Compare January 2, 2024 18:44

mwcraig approved these changes Jan 2, 2024

View reviewed changes

mwcraig merged commit 78b4b7e into feder-observatory:main Jan 2, 2024
9 checks passed

Tanner728 deleted the exoplanet branch January 2, 2024 19:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Exoplanet Object #235

Add Exoplanet Object #235

Tanner728 commented Dec 26, 2023 •

edited by mwcraig

Loading

codecov-commenter commented Dec 26, 2023 •

edited

Loading

mwcraig left a comment

mwcraig Dec 26, 2023

Tanner728 Dec 27, 2023

mwcraig Dec 27, 2023

mwcraig Dec 27, 2023 •

edited

Loading

mwcraig Dec 27, 2023

mwcraig commented Dec 27, 2023

Tanner728 commented Dec 27, 2023

mwcraig commented Dec 27, 2023

mwcraig Dec 27, 2023

Tanner728 Dec 27, 2023

mwcraig Dec 27, 2023

mwcraig Dec 27, 2023

mwcraig commented Dec 27, 2023

Tanner728 commented Dec 27, 2023

mwcraig commented Dec 27, 2023

mwcraig Dec 27, 2023

mwcraig Dec 31, 2023

Tanner728 Dec 31, 2023

mwcraig Jan 1, 2024

Tanner728 commented Jan 1, 2024

mwcraig commented Jan 1, 2024

mwcraig commented Jan 1, 2024

mwcraig commented Jan 2, 2024

mwcraig commented Jan 2, 2024

mwcraig left a comment

Tanner728 commented Jan 2, 2024

	from stellarphot.settings.models import ApertureSettings,Exoplanet
	from stellarphot.settings.models import ApertureSettings, Exoplanet

		assert u.get_physical_type(planet.duration) == "time"


		@pytest.mark.parametrize("bad_one", [

Add Exoplanet Object #235

Add Exoplanet Object #235

Conversation

Tanner728 commented Dec 26, 2023 • edited by mwcraig Loading

codecov-commenter commented Dec 26, 2023 • edited Loading

Codecov Report

mwcraig 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

mwcraig Dec 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwcraig commented Dec 27, 2023

Tanner728 commented Dec 27, 2023

mwcraig commented Dec 27, 2023

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 27, 2023

Tanner728 commented Dec 27, 2023

mwcraig commented Dec 27, 2023

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

Tanner728 commented Jan 1, 2024

mwcraig commented Jan 1, 2024

mwcraig commented Jan 1, 2024

mwcraig commented Jan 2, 2024

mwcraig commented Jan 2, 2024

mwcraig left a comment

Choose a reason for hiding this comment

Tanner728 commented Jan 2, 2024

Tanner728 commented Dec 26, 2023 •

edited by mwcraig

Loading

codecov-commenter commented Dec 26, 2023 •

edited

Loading

mwcraig Dec 27, 2023 •

edited

Loading