Add `SourceField` and subclasses, rework `Source` #405

teutoburg · 2024-04-18T11:57:32Z

Some ~~minor~~ major refactoring in vague preparation of scopesim-targets...

Main changes

Introduce SourceField and subclasses: As discussed in the Waidhofen hackathon, this family of classes will now hold the source fields as part of the Source class. Different subclasses exist for different kinds of fields (table, image, cube). This produced a bit of a rat-tail of changes elsewhere, but should ultimately move us forward.
Make Source.spectra (now just a collection of all field.spectra) a dict. This was a rather minimal change with limited need to adapt elsewhere, but is a lot more explicit on which spectra are referenced where, instead of hoping the the indices of the previous list don't get mixed up when changing stuff in the source.

Other changes

Added the SED table parser convenience function for use in the METIS notebooks.
Some other refactoring, mostly in the Source class.

Further notes

Ignore all the force pushes, which resulted from rebasing...

~~This needs 3.10, so waiting for that...~~

codecov · 2024-04-18T12:05:51Z

Codecov Report

Attention: Patch coverage is 79.57560% with 77 lines in your changes missing coverage. Please review.

Project coverage is 76.54%. Comparing base (2260ac3) to head (7ac96b8).
Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
scopesim/source/source.py	78.35%	29 Missing ⚠️
scopesim/source/source_fields.py	85.33%	22 Missing ⚠️
scopesim/source/source_utils.py	70.14%	20 Missing ⚠️
scopesim/effects/spectral_efficiency.py	0.00%	4 Missing ⚠️
scopesim/effects/ter_curves.py	71.42%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #405      +/-   ##
==========================================
+ Coverage   75.97%   76.54%   +0.57%     
==========================================
  Files          64       65       +1     
  Lines        7820     7965     +145     
==========================================
+ Hits         5941     6097     +156     
+ Misses       1879     1868      -11

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

This pickle functionality wasn't used anywhere Keep it around for now in case I was mistakten. To be replaces with a YAML-based solution in the future... Early raise... No more np.all

Fix broken cube test, cube stuff and plotting Minor stuff and more units, fix failing tests

hugobuddel · 2024-08-30T09:00:50Z

Could you please summarize the idea behind these changes so it is recorded somewhere? I didn't follow the full discussion at Waidhofen, so I'll trust you on that.

teutoburg · 2024-08-30T19:16:06Z

Could you please summarize the idea behind these changes so it is recorded somewhere? I didn't follow the full discussion at Waidhofen, so I'll trust you on that.

Added this information as a module docstring in source_fields.py, which should (eventually) get pulled into the documentation build, that way it's also available there (including the class diagram, which can be rendered with the right Sphinx plugin).

teutoburg · 2024-09-01T11:56:55Z

Class diagram from the module docstring, because it doesn't yet render anywhere else:

classDiagram
class SourceField{+field}
class SpectrumSourceField{+spectra}
class HDUSourceField{+wcs}
SourceField <|-- SpectrumSourceField
SourceField <|-- HDUSourceField
SpectrumSourceField <|-- TableSourceField
SpectrumSourceField <|-- ImageSourceField
HDUSourceField <|-- ImageSourceField
HDUSourceField <|-- CubeSourceField

scopesim/source/source.py

hugobuddel · 2024-09-03T07:38:56Z

ScopeSim_Templates is now broken:

ScopeSim_Templates$ python -m pytest
============================================== test session starts ==============================================
platform linux -- Python 3.11.5, pytest-8.2.1, pluggy-1.5.0
rootdir: .../ScopeSim_Templates
configfile: pyproject.toml
plugins: anyio-3.7.1, cov-4.1.0
collected 82 items / 1 error                                                                                    

==================================================== ERRORS =====================================================
_________________ ERROR collecting scopesim_templates/tests/test_utils/test_validate_content.py _________________
scopesim_templates/tests/test_utils/test_validate_content.py:16: in <module>
    spiral_two_component(),
scopesim_templates/utils/general_utils.py:74: in wrapper
    src = func(*args, **kwargs)
scopesim_templates/extragalactic/galaxies.py:303: in spiral_two_component
    src._meta_dicts += [{}] * (len(src.fields) - len(src._meta_dicts))
../ScopeSim/scopesim/source/source.py:416: in _meta_dicts
    return [fld.meta for fld in self.fields]
../ScopeSim/scopesim/source/source.py:416: in <listcomp>
    return [fld.meta for fld in self.fields]
E   AttributeError: 'ImageHDU' object has no attribute 'meta'
------------------------------------------------ Captured stdout ------------------------------------------------
astar.scopesim.source.source - ERROR: spectra setting is deprecated
============================================ short test summary info ============================================
ERROR scopesim_templates/tests/test_utils/test_validate_content.py - AttributeError: 'ImageHDU' object has no attribute 'meta'
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
=============================================== 1 error in 4.85s ================================================

Could you please either make this PR (even more) backwards compatible or otherwise create a sibling-PR for ScopeSim_Templates?

hugobuddel · 2024-09-03T07:39:44Z

Did you verify that everything in the IRDB functions with the new SourceField classes?

hugobuddel · 2024-09-03T08:00:25Z

I gathered from the Waidhofen meeting that we (later) want to support more parametric sources. E.g. an 'image' that might not have a pixel array associated with it. How would that fit in this scheme?

hugobuddel

Thanks @teutoburg! ScopeSim keeps getting better! The decoupling between field/spectra/meta was pretty painful to work with (e.g. that [{}] assert...), now it is much better and more extensible.

Please judge for yourself how to deal with my comments. There are many, but most of them are pretty minor, and some of the larger ones might be postponed.

hugobuddel · 2024-09-03T06:51:36Z

scopesim/effects/ter_curves.py

+                if isinstance(fld, CubeSourceField):
+                    fld.field = apply_throughput_to_cube(fld.field, thru)
+                    continue
+
+                for isp, spec in fld.spectra.items():
+                    fld.spectra[isp] = combine_two_spectra(
+                        spec, thru, "multiply", wave_min, wave_max)


I find this a bit risky, as we haven't checked whether fld has a spectra attribute. Maybe

Suggested change

if isinstance(fld, CubeSourceField):

fld.field = apply_throughput_to_cube(fld.field, thru)

continue

for isp, spec in fld.spectra.items():

fld.spectra[isp] = combine_two_spectra(

spec, thru, "multiply", wave_min, wave_max)

if isinstance(fld, CubeSourceField):

fld.field = apply_throughput_to_cube(fld.field, thru)

elif isinstance(fld, SpectrumSourceField):

fld.spectra = {

isp: combine_two_spectra(spec, thru, "multiply", wave_min, wave_max)

for isp, spec in fld.spectra.items()

}

scopesim/effects/fits_headers.py

scopesim/effects/ter_curves_utils.py

scopesim/optics/fov_utils.py

scopesim/source/source.py

hugobuddel · 2024-09-03T08:08:00Z

scopesim/source/source_fields.py

+    def _write_stream(self, stream: TextIO) -> None:
+        raise NotImplementedError("Subclasses should implement this.")


See comment on Source.__repr__(). I believe _write_stream() is not necessary.

scopesim/source/source_fields.py

hugobuddel · 2024-09-03T08:46:20Z

scopesim/source/source_utils.py

+        if (isinstance(spectra, Iterable) and
+                not isinstance(spectra, np.ndarray)):
+            if all(isinstance(spec, SourceSpectrum) for spec in spectra):
+                yield from spectra


I don't really like this refactoring, because the code is more complex and the yield statements indicate that it works with generators but it does not, because the all() would have used up everything. E.g.

from itertools import count, takewhile numbers = takewhile(lambda x: x< 20, count(10)) def convert(spectra): def get_list(): if all(isinstance(spec, int) for spec in spectra): yield from spectra return list(get_list()) list_of_numbers = convert(numbers) print(f"{list_of_numbers=}")

will print

list_of_numbers=[]

I think it is better to simplify this by converting the input to a list and remove the yields.

hugobuddel · 2024-09-03T08:52:16Z

scopesim/source/source_utils.py

-
-    if isinstance(wave_min, u.Quantity):
-        wave_min = wave_min.to(u.Angstrom).value
-    else:
-        wave_min *= 1E4
-
-    if isinstance(wave_max, u.Quantity):
-        wave_max = wave_max.to(u.Angstrom).value
-    else:
-        wave_max *= 1E4
+    # Note: Assuming um if given as float.
+    wave_min = (wave_min << u.um << u.Angstrom).value
+    wave_max = (wave_max << u.um << u.Angstrom).value
+    # Note: There appear to be some float shenanigans going on here, but
+    # rounding produces an error in the spectrum evaluation. Not sure what's
+    # going on, maybe it's fine as-is.


This is probably NOT fine as it is. The problem is:

>>> (1 << u.um << u.Angstrom).value 9999.999999999998

The 1E4 conversion was ugly, but correct.

It might be that in this particular case everything is fine, but I think we should not rely on that.

This incorrect conversion also causes all kinds of problems with synphot.

I've reconsidered. We should make this change.

This unit conversion could indeed go wrong. However, it already would go wrong if fractional wavelengths are used, e.g. wavemin=1333.3. Our code should not depend on float values being integer.

This means that even if we would hardcode this 1E4 here (and similarly elsewhere), that the code will still be broken in general. So we should not use such hacks and instead just let astropy handle it, as you did.

Per extension, it should not cause any problems if a value of 1000.0 becomes 999.9999999999 or 1000.000000001:

Any (synphot) code that computes overlaps should be resilient to such rounding errors and just ignore it.

Any plotting/printing code should just print 1000.0. (At least for UX, not for debugging.)

Perhaps we should find some floating point numbers that you cannot faithfully multiply/divide by (e.g.) 1E4. Maybe one number that rounds up and one that rounds down. Then we should use those in the basic_instrument test package to detect any errors that such a conversion causes. Or maybe have some test irdb packages that uses weird units like wavelengths in parsec and such.

hugobuddel · 2024-09-03T09:28:18Z

ScopeSim_Templates is now broken:
[...]
Could you please either make this PR (even more) backwards compatible or otherwise create a sibling-PR for ScopeSim_Templates?

Oh I'm being stupid, I didn't notice AstarVienna/ScopeSim_Templates#91 and I was on a branch so git pull didn't fetch it.

scopesim/source/source.py

Co-authored-by: Hugo Buddelmeijer <[email protected]>

Some code (mostly in ScopeSim_Templates) relies on single-field sources exposing their field meta as the Source's meta. If there's more than one field, the metas should always be accessed through the fields to avoid ambiguity. This (hacky) solution now seems to work and is certainly better than silently always returning just the first field's meta. Also removed the setter for meta. It's a dict, so if it needs to be updated, just do that. It should never be replaced (== overwritten!) anyway.

teutoburg self-assigned this Apr 18, 2024

teutoburg added the refactor Implementation improvement label Apr 18, 2024

teutoburg force-pushed the fh/sourcestuff branch from 75f6f52 to 85fa9ca Compare May 22, 2024 12:18

teutoburg force-pushed the fh/sourcestuff branch from 85fa9ca to 4068914 Compare May 31, 2024 10:02

teutoburg force-pushed the fh/sourcestuff branch 2 times, most recently from dad621f to 428f959 Compare August 28, 2024 21:04

teutoburg changed the title ~~Fh/sourcestuff~~ Add SourceField and subclasses, rework Source Aug 28, 2024

teutoburg added 5 commits August 29, 2024 11:49

Some refactoring and several minor changes, see below...

50ba976

This pickle functionality wasn't used anywhere Keep it around for now in case I was mistakten. To be replaces with a YAML-based solution in the future... Early raise... No more np.all

Make Source.spectra a dict, fix aftermath

48d027f

Introduce SourceField and subclasses

2f9ff8c

Fix broken cube test, cube stuff and plotting Minor stuff and more units, fix failing tests

Adapt to introduction of SourceFields

9e5dfb7

Add SED table parser for notebook

613454d

teutoburg force-pushed the fh/sourcestuff branch from 428f959 to 613454d Compare August 29, 2024 09:49

teutoburg marked this pull request as ready for review August 29, 2024 10:34

teutoburg requested review from hugobuddel and astronomyk August 29, 2024 10:34

teutoburg mentioned this pull request Aug 29, 2024

Accept both pre-v0.9 and current ScopeSim versions AstarVienna/ScopeSim_Templates#91

Merged

Add module docstring

b98401c

hugobuddel reviewed Sep 1, 2024

View reviewed changes

scopesim/source/source.py Outdated Show resolved Hide resolved

hugobuddel requested changes Sep 3, 2024

View reviewed changes

hugobuddel reviewed Sep 3, 2024

View reviewed changes

scopesim/source/source.py Outdated Show resolved Hide resolved

Spelling

ca2842c

Co-authored-by: Hugo Buddelmeijer <[email protected]>

teutoburg and others added 9 commits September 9, 2024 16:06

Remove forgotten debug printing

0f4058a

Co-authored-by: Hugo Buddelmeijer <[email protected]>

Forgotten argument and more spelling

0ffd8da

Just remove this _meta_dicts thing

0683306

Add comment about problematic specrefoffset

e05d3c5

Add to docstring and comment in fov_utils.py

12fc225

Add from_hdul kwarg to CubeField instead of messy meta :P

5d7f74f

Fix default value for WCS

c985fa4

Increase coverage for cube field

7ac96b8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `SourceField` and subclasses, rework `Source` #405

Add `SourceField` and subclasses, rework `Source` #405

teutoburg commented Apr 18, 2024 •

edited

Loading

codecov bot commented Apr 18, 2024 •

edited

Loading

hugobuddel commented Aug 30, 2024

teutoburg commented Aug 30, 2024

teutoburg commented Sep 1, 2024

hugobuddel commented Sep 3, 2024 •

edited

Loading

hugobuddel commented Sep 3, 2024

hugobuddel commented Sep 3, 2024

hugobuddel left a comment

hugobuddel Sep 3, 2024

hugobuddel Sep 3, 2024

hugobuddel Sep 3, 2024

hugobuddel Sep 3, 2024

hugobuddel Sep 3, 2024

hugobuddel commented Sep 3, 2024

		def _write_stream(self, stream: TextIO) -> None:
		raise NotImplementedError("Subclasses should implement this.")

Add SourceField and subclasses, rework Source #405

Are you sure you want to change the base?

Add SourceField and subclasses, rework Source #405

Conversation

teutoburg commented Apr 18, 2024 • edited Loading

Main changes

Other changes

Further notes

codecov bot commented Apr 18, 2024 • edited Loading

Codecov Report

hugobuddel commented Aug 30, 2024

teutoburg commented Aug 30, 2024

teutoburg commented Sep 1, 2024

hugobuddel commented Sep 3, 2024 • edited Loading

hugobuddel commented Sep 3, 2024

hugobuddel commented Sep 3, 2024

hugobuddel left a comment

Choose a reason for hiding this comment

hugobuddel Sep 3, 2024

Choose a reason for hiding this comment

hugobuddel Sep 3, 2024

Choose a reason for hiding this comment

hugobuddel Sep 3, 2024

Choose a reason for hiding this comment

hugobuddel Sep 3, 2024

Choose a reason for hiding this comment

hugobuddel Sep 3, 2024

Choose a reason for hiding this comment

hugobuddel commented Sep 3, 2024

Add `SourceField` and subclasses, rework `Source` #405

Add `SourceField` and subclasses, rework `Source` #405

teutoburg commented Apr 18, 2024 •

edited

Loading

codecov bot commented Apr 18, 2024 •

edited

Loading

hugobuddel commented Sep 3, 2024 •

edited

Loading