Add SourceField and subclasses, rework Source

[teutoburg]

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]

Codecov Report

Attention: Patch coverage is 79.52756% with 78 lines in your changes missing coverage. Please review.

Project coverage is 76.53%. Comparing base (2260ac3) to head (4c9e3c5).
Report is 186 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.58%	20 Missing ⚠️
scopesim/effects/spectral_efficiency.py	0.00%	4 Missing ⚠️
scopesim/effects/ter_curves.py	71.42%	2 Missing ⚠️
scopesim/optics/optical_train.py	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #405      +/-   ##
==========================================
+ Coverage   75.97%   76.53%   +0.56%     
==========================================
  Files          64       65       +1     
  Lines        7820     7970     +150     
==========================================
+ Hits         5941     6100     +159     
+ Misses       1879     1870       -9     

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

[hugobuddel]

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]

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]

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

Loading

[hugobuddel]

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]

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

[hugobuddel]

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]

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

[hugobuddel]

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.

[teutoburg]

We should really properly test cube somewhere (other than the notebooks). Running LSS-YSO_model_simulation and LSS_AGN-01_preparation from this branch revealed that apply_throughput_to_cube is indeed broken because the shape of the cube changes and so the original WCS no longer fits. So the whole idea of keeping the cube WCS around is actually not working. Would have been nice to notice that much sooner (like when I originally started this branch in May I think). Anyway, it's certainly fixable, but will add another delay to this. Also, why wasn't this caught by the "notebook tests" that run here??

[teutoburg]

Also, why wasn't this caught by the "notebook tests" that run here??

Ah, because that only runs the notebooks in the ScopeSim repo itself, not the IRDB ones. Is that intentional?

[hugobuddel]

Also, why wasn't this caught by the "notebook tests" that run here??

Ah, because that only runs the notebooks in the ScopeSim repo itself, not the IRDB ones. Is that intentional?

Yes the ScopeSim test only runs the notebooks in ScopeSim itself, as I thought it would be best that (as a default) each project only runs their own tests/notebooks.

ScopeSim_Data is the only exception, it runs all the tests and notebooks of all our projects, because it has to do so in order to fetch the files those tests and notebooks need. And ScopeSim_Data thereby doubles as a full end-to-end test.

Hence my question whether everything in the IRDB still works.

Ultimately it would be great if we have some (minimal) tests in ScopeSim proper, and only use the IRDB notebooks as integration tests.

[hugobuddel]

I'll assume your latest commit fixes the IRDB notebooks. The things open are al minor, so I'll let you decide when this is ready to merge.

[teutoburg]

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?

To also address this before finally merging: My current idea (might change as things develop) is that the Source class and by extension the source field classes will serve as the interface between the target description and the optical train. That is, ScopeSim at some point needs a "pixel array" from the source. When and how the source will be created based on a parametrized target description is TBD, but will most likely take into account factors like pixel and/or plate scales of the optical train.

[teutoburg]

@hugobuddel thanks for the extensive review process! I know it took me a while to react to all of it (or indeed most of it, I'm leaving the less important repr stuff for discussion at a later point, because that's more cosmetical), but it did point me to a few things that I would have missed otherwise and that would have come back to bite me later.

Now, some 4 months after initially starting this refactor process, I'm happy to finally press the big green button 🚀

[hugobuddel]

Great! I learned quite a bit about our code base too.