ccd_tempreture taking astropy unit #150

Abinash-bit · 2024-11-29T05:37:49Z

PR Description

Function Signature Update:

Changed ccd_temperature=-60.0 to ccd_temperature=-60.0 * u.deg_C.

Temperature Handling:

Added a check to ensure ccd_temperature is an Astropy Quantity.

Converted non-Quantity inputs to u.deg_C.

Ensured temperature is in degrees Celsius for consistency.

Gain Interpolation:

Used temp_x values directly (assumed in degrees Celsius).

Used .value to get numerical values for interpolation.

Docstring Update:

Described ccd_temperature as an Astropy Quantity with units.

Testing:

The function now accepts both Quantity and float inputs for temperature.

Temperatures provided in different units are converted to degrees Celsius.

This fixes #77

nabobalis

This will need to make sure there is a unit test for this and a changelog entry.

nabobalis · 2024-11-29T22:01:37Z

sunkit_instruments/suvi/suvi.py

    """
-    Get the SUVI instrument response for a specific wavelength channel,
-    spacecraft, CCD temperature, and exposure type.
+    Get the SUVI instrument response for a specific wavelength channel, spacecraft, CCD temperature, and exposure type.


Suggested change

Get the SUVI instrument response for a specific wavelength channel, spacecraft, CCD temperature, and exposure type.

Get the SUVI instrument response for a specific wavelength channel, spacecraft,

CCD temperature, and exposure type.

nabobalis · 2024-11-29T22:01:52Z

sunkit_instruments/suvi/suvi.py

-    parameters are read automatically from the metadata, or the parameters
-    can be passed manually, with ``request`` specifying the desired wavelength
-    channel.
+    ``request`` can either be an L1b filename (FITS or netCDF), in which case all of those parameters are read automatically from the metadata, or the parameters can be passed manually, with ``request`` specifying the desired wavelength channel.


Suggested change

``request`` can either be an L1b filename (FITS or netCDF), in which case all of those parameters are read automatically from the metadata, or the parameters can be passed manually, with ``request`` specifying the desired wavelength channel.

``request`` can either be an L1b filename (FITS or netCDF), in which case all of those parameters

are read automatically from the metadata, or the parameters can be passed manually, with ``request`` specifying the desired wavelength channel.

nabobalis · 2024-11-29T22:02:45Z

sunkit_instruments/suvi/suvi.py

-    ccd_temperature: `float`, optional.
-        The CCD temperature, in degrees Celsius, default is -60.
+    ccd_temperature : astropy.units.Quantity, optional
+        The CCD temperature, in degrees Celsius, default is -60.0 * u.deg_C.


Suggested change

The CCD temperature, in degrees Celsius, default is -60.0 * u.deg_C.

The CCD temperature, in degrees Celsius, default is ``-60.0 * u.deg_C``.

Please do not resolve comments unless they are fixed.

Co-authored-by: Nabil Freij <[email protected]>

nabobalis · 2024-11-30T17:58:01Z

changelog/150.feature.3.rst

+### Added
+- Support for `ccd_temperature` as an Astropy Quantity in the `get_response` function.
+### Changed
+- Updated the `get_response` function signature to accept `ccd_temperature` as an Astropy Quantity.
+- Modified the function to handle `ccd_temperature` inputs with and without units, converting them to degrees Celsius.
+- Updated the docstring of the `get_response` function to reflect the new parameter type and units requirement.


This changelog is too verbose.

sunkit_instruments/suvi/suvi.py

sunkit_instruments/suvi/tests/test_suvi.py

Abinash-bit · 2024-12-02T03:52:09Z

In the last 2 commits 4 checks are failing and most of the tests are skipped , how can we proceed?

nabobalis · 2024-12-02T04:07:42Z

In the last 2 commits 4 checks are failing and most of the tests are skipped , how can we proceed?

We check why they are failing and address that problem.

The error is

../../.tox/py312/lib/python3.12/site-packages/sunkit_instruments/suvi/__init__.py:2: in <module>
    from .suvi import *  # NOQA
../../.tox/py312/lib/python3.12/site-packages/sunkit_instruments/suvi/suvi.py:8: in <module>
    from astropy.utils.decorators import quantity_input
E   ImportError: cannot import name 'quantity_input' from 'astropy.utils.decorators' (/home/runner/work/sunkit-instruments/sunkit-instruments/.tox/py312/lib/python3.12/site-packages/astropy/utils/decorators.py)

I would recommend checking the failed logs more often or at least running the unit test suite locally before you push changes.

…can debug

Abinash-bit · 2024-12-04T16:42:26Z

I think we are left with test_suvi.py and the data files that is L1B_NC and L1B_FITS.

nabobalis · 2024-12-04T17:17:29Z

I think we are left with test_suvi.py and the data files that is L1B_NC and L1B_FITS.

You mean adding more tests?

nabobalis · 2024-12-04T17:17:49Z

sunkit_instruments/suvi/suvi.py

@@ -23,7 +23,6 @@

 PATH_TO_FILES = Path(__file__).parent / "data"

-


Revert this

nabobalis · 2024-12-04T17:17:54Z

sunkit_instruments/suvi/suvi.py

@@ -46,7 +45,6 @@ def _despike(image, dqf_mask, filter_width):
    despiked_image[indices] = image_gaussian_filtered[indices]
    return despiked_image

-


Revert this

nabobalis · 2024-12-04T17:17:58Z

sunkit_instruments/suvi/suvi.py

@@ -78,7 +76,6 @@ def despike_l1b_file(filename, filter_width=7):
    despiked_image = _despike(image, dqf_mask, filter_width)
    return sunpy.map.Map(despiked_image, header)

-


Revert this

changelog/150.feature.3.rst

Co-authored-by: Nabil Freij <[email protected]>

Abinash-bit · 2024-12-04T17:50:23Z

I think we are left with test_suvi.py and the data files that is L1B_NC and L1B_FITS.

You mean adding more tests?

The test file says checks are skipped and it is skipped because the file can not get the L1B_NC and L1B_FITS files may be not sure.

Abinash-bit · 2024-12-07T07:34:11Z

What happened to the unit changes?

Sorry could not get you, I have changed where ever there is a change you have requested.

But you also undid all of the changes which were the point of this pull request?

yes i know , i am sorry, changing again.

Now please suggest any changes.

sunkit_instruments/suvi/suvi.py

nabobalis · 2024-12-07T07:35:08Z

sunkit_instruments/suvi/tests/test_suvi.py

-
-

These lines should be added back.

nabobalis · 2024-12-07T07:35:21Z

sunkit_instruments/suvi/tests/test_suvi.py

    l1b_nc_response = suvi.get_response(L1B_NC)
    assert l1b_nc_response["wavelength_channel"] == 171
+    assert l1b_nc_response["ccd_temperature"] == ccd_temp_avg


Suggested change

assert l1b_nc_response["ccd_temperature"] == ccd_temp_avg

assert l1b_nc_response["ccd_temperature"] == (header["CCD_TMP1"] + header["CCD_TMP2"]) / 2.0 * u.deg_C

nabobalis · 2024-12-07T07:35:27Z

sunkit_instruments/suvi/tests/test_suvi.py

@@ -24,15 +24,33 @@ def test_suvi_despiking_nc(L1B_NC):


 def test_get_response_nc(L1B_NC):
+    header, _, _ = suvi.read_suvi(L1B_NC)
+    ccd_temp_avg = (header["CCD_TMP1"] + header["CCD_TMP2"]) / 2.0 * u.deg_C


Suggested change

ccd_temp_avg = (header["CCD_TMP1"] + header["CCD_TMP2"]) / 2.0 * u.deg_C

nabobalis · 2024-12-07T07:35:41Z

sunkit_instruments/suvi/tests/test_suvi.py

    l1b_fits_response = suvi.get_response(L1B_FITS)
    assert l1b_fits_response["wavelength_channel"] == 171
+    assert l1b_fits_response["ccd_temperature"] == ccd_temperature


Suggested change

assert l1b_fits_response["ccd_temperature"] == ccd_temperature

assert l1b_fits_response["ccd_temperature"] == (header["CCD_TMP1"] + header["CCD_TMP2"]) / 2.0 * u.deg_C

nabobalis · 2024-12-07T07:35:47Z

sunkit_instruments/suvi/tests/test_suvi.py



 def test_get_response_fits(L1B_FITS):
+    header, _, _ = suvi.read_suvi(L1B_FITS)
+    ccd_temperature = (header["CCD_TMP1"] + header["CCD_TMP2"]) / 2.0 * u.deg_C


Suggested change

ccd_temperature = (header["CCD_TMP1"] + header["CCD_TMP2"]) / 2.0 * u.deg_C

nabobalis · 2024-12-07T07:38:09Z

Now please suggest any changes.

I have a few more comments but there are some older ones still left unresolved.

Co-authored-by: Nabil Freij <[email protected]>

Abinash-bit · 2024-12-07T14:08:30Z

As per the old comments:

You have asked why there is .3. in the changelog file name? which i have commented.
You have asked about quantity decorators , i have added.
The docstring comments are nearly fixed.
Asked about ccd_temp_avg = (header["CCD_TMP1"] + header["CCD_TMP2"]) / 2.0 this line which is fixed by your suggested snippet.

I think the comments are addressed you can check once you get time.

nabobalis · 2024-12-07T16:53:21Z

changelog/150.feature.3.rst

Delete this file.

nabobalis · 2024-12-07T16:56:39Z

sunkit_instruments/suvi/suvi.py

@@ -156,7 +156,8 @@ def get_response(request, spacecraft=16, ccd_temperature=-60.0, exposure_type="l
        header, _, _ = read_suvi(request)
        wavelength_channel = int(header["WAVELNTH"])
        spacecraft = int(header["TELESCOP"].replace(" ", "").replace("G", ""))
-        ccd_temperature = (header["CCD_TMP1"] + header["CCD_TMP2"]) / 2.0
+        ccd_temp_avg = (header["CCD_TMP1"] + header["CCD_TMP2"]) / 2.0
+        ccd_temperature = ccd_temp_avg * u.deg_C


Why do we redefine ccd_temperature here?

ccd_tempreture taking astropy unit

eda4374

nabobalis reviewed Nov 29, 2024

View reviewed changes

Abinash-bit and others added 8 commits November 30, 2024 18:36

Update sunkit_instruments/suvi/suvi.py

7b4f3f3

Co-authored-by: Nabil Freij <[email protected]>

Update sunkit_instruments/suvi/suvi.py

9625f71

Co-authored-by: Nabil Freij <[email protected]>

Update sunkit_instruments/suvi/suvi.py

33d13fa

Co-authored-by: Nabil Freij <[email protected]>

Adding unit tests for get_response

85f3ff6

updates

5ddc1b0

Fixing precommit

752a01d

Adding the changelog

fc20b49

again fixing the precommit

fcbd964

nabobalis reviewed Nov 30, 2024

View reviewed changes

sunkit_instruments/suvi/suvi.py Outdated Show resolved Hide resolved

nabobalis reviewed Nov 30, 2024

View reviewed changes

sunkit_instruments/suvi/tests/test_suvi.py Show resolved Hide resolved

Abinash-bit added 2 commits December 1, 2024 12:08

Adding the quantity decorators and removing the manual checks

55acee0

Correcting changelog

7e74533

Abinash-bit added 5 commits December 4, 2024 19:54

Coming back to the code base where test cases did not fail so that i …

832b89c

…can debug

Just Changes

2277451

updates

119f05b

May be proceeding towards final script

7b0e0e8

Fixing pre-commit

e5c8160

nabobalis reviewed Dec 4, 2024

View reviewed changes

sunkit_instruments/suvi/suvi.py Outdated

@@ -23,7 +23,6 @@

PATH_TO_FILES = Path(__file__).parent / "data"

Copy link

Contributor

nabobalis Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert this

nabobalis reviewed Dec 4, 2024

View reviewed changes

changelog/150.feature.3.rst Outdated Show resolved Hide resolved

Update changelog/150.feature.3.rst

5da93f3

Co-authored-by: Nabil Freij <[email protected]>

Abinash-bit added 2 commits December 7, 2024 12:56

fixing codestyle and resolving commits

e1b7abf

precommit-fix

c2d1899

nabobalis reviewed Dec 7, 2024

View reviewed changes

sunkit_instruments/suvi/suvi.py Outdated Show resolved Hide resolved

nabobalis reviewed Dec 7, 2024

View reviewed changes

sunkit_instruments/suvi/tests/test_suvi.py Outdated

Comment on lines 8 to 9

Copy link

Contributor

nabobalis Dec 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These lines should be added back.

nabobalis reviewed Dec 7, 2024

View reviewed changes

Abinash-bit and others added 14 commits December 7, 2024 13:11

Update sunkit_instruments/suvi/suvi.py

c98c627

Co-authored-by: Nabil Freij <[email protected]>

Adding line space in test_suvi.py

6084b6c

resolving conflicts

eb45e53

Update sunkit_instruments/suvi/tests/test_suvi.py

64148c9

Co-authored-by: Nabil Freij <[email protected]>

Update sunkit_instruments/suvi/tests/test_suvi.py

f2bf28a

Co-authored-by: Nabil Freij <[email protected]>

Update sunkit_instruments/suvi/tests/test_suvi.py

9436955

Co-authored-by: Nabil Freij <[email protected]>

Update sunkit_instruments/suvi/tests/test_suvi.py

c203be7

Co-authored-by: Nabil Freij <[email protected]>

precommit-fix

26a0548

resolving conflicts

4337ea4

addressing old comments

89e9221

Update sunkit_instruments/suvi/suvi.py

35e03e0

Co-authored-by: Nabil Freij <[email protected]>

addressing old comments

91063ca

Resolving conflicts

85689ee

Update sunkit_instruments/suvi/suvi.py

f0d5c8d

Co-authored-by: Nabil Freij <[email protected]>

Abinash-bit added 3 commits December 7, 2024 19:47

just precommit

8fd73da

resolving conflicts

70d6339

Changing the name of the changelog

c99b66f

nabobalis reviewed Dec 7, 2024

View reviewed changes

changelog/150.feature.3.rst

Copy link

Contributor

nabobalis Dec 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete this file.

nabobalis reviewed Dec 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ccd_tempreture taking astropy unit #150

ccd_tempreture taking astropy unit #150

Abinash-bit commented Nov 29, 2024

nabobalis left a comment

nabobalis Nov 29, 2024

nabobalis Nov 29, 2024

nabobalis Nov 29, 2024

nabobalis Dec 4, 2024

nabobalis Nov 30, 2024

Abinash-bit commented Dec 2, 2024

nabobalis commented Dec 2, 2024 •

edited

Loading

Abinash-bit commented Dec 4, 2024

nabobalis commented Dec 4, 2024

nabobalis Dec 4, 2024

nabobalis Dec 4, 2024

nabobalis Dec 4, 2024

Abinash-bit commented Dec 4, 2024

Abinash-bit commented Dec 7, 2024

nabobalis Dec 7, 2024

nabobalis Dec 7, 2024

nabobalis Dec 7, 2024

nabobalis Dec 7, 2024

nabobalis Dec 7, 2024

nabobalis commented Dec 7, 2024

Abinash-bit commented Dec 7, 2024

nabobalis Dec 7, 2024

nabobalis Dec 7, 2024

	Get the SUVI instrument response for a specific wavelength channel, spacecraft, CCD temperature, and exposure type.
	Get the SUVI instrument response for a specific wavelength channel, spacecraft,
	CCD temperature, and exposure type.

	``request`` can either be an L1b filename (FITS or netCDF), in which case all of those parameters are read automatically from the metadata, or the parameters can be passed manually, with ``request`` specifying the desired wavelength channel.
	``request`` can either be an L1b filename (FITS or netCDF), in which case all of those parameters
	are read automatically from the metadata, or the parameters can be passed manually, with ``request`` specifying the desired wavelength channel.

	The CCD temperature, in degrees Celsius, default is -60.0 * u.deg_C.
	The CCD temperature, in degrees Celsius, default is ``-60.0 * u.deg_C``.

		@@ -23,7 +23,6 @@

		PATH_TO_FILES = Path(__file__).parent / "data"

		@@ -46,7 +45,6 @@ def _despike(image, dqf_mask, filter_width):
		despiked_image[indices] = image_gaussian_filtered[indices]
		return despiked_image

		@@ -78,7 +76,6 @@ def despike_l1b_file(filename, filter_width=7):
		despiked_image = _despike(image, dqf_mask, filter_width)
		return sunpy.map.Map(despiked_image, header)

	assert l1b_nc_response["ccd_temperature"] == ccd_temp_avg
	assert l1b_nc_response["ccd_temperature"] == (header["CCD_TMP1"] + header["CCD_TMP2"]) / 2.0 * u.deg_C

	assert l1b_fits_response["ccd_temperature"] == ccd_temperature
	assert l1b_fits_response["ccd_temperature"] == (header["CCD_TMP1"] + header["CCD_TMP2"]) / 2.0 * u.deg_C

ccd_tempreture taking astropy unit #150

Are you sure you want to change the base?

ccd_tempreture taking astropy unit #150

Conversation

Abinash-bit commented Nov 29, 2024

PR Description

nabobalis 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

Abinash-bit commented Dec 2, 2024

nabobalis commented Dec 2, 2024 • edited Loading

Abinash-bit commented Dec 4, 2024

nabobalis commented Dec 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Abinash-bit commented Dec 4, 2024

Abinash-bit commented Dec 7, 2024

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

nabobalis commented Dec 7, 2024

Abinash-bit commented Dec 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nabobalis commented Dec 2, 2024 •

edited

Loading