Make DataContainer an attribute of Effect
#482
Conversation
This used to be the base class. Composition over inheritance makes sense in this particular case. An Effect HAS data, rather than BEING data.
teutoburg
added
refactor
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #482 +/- ##
==========================================
+ Coverage 76.58% 76.69% +0.10%
==========================================
Files 66 66
Lines 8094 8118 +24
==========================================
+ Hits 6199 6226 +27
+ Misses 1895 1892 -3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
I think we should ensure that the IRDB notebooks run without problem using this branch. Could you please check those notebooks @teutoburg ?
| self.data_container = DataContainer(filename=filename, **kwargs) | ||
| self.meta = kwargs.get("meta", {}) | ||
| self.cmds = kwargs.get("cmds") | ||
|
|
||
| self.meta.update(self.data_container.meta) | ||
| self.meta["z_order"] = [] | ||
| self.meta["include"] = True | ||
| self.meta.update(kwargs) |
There was a problem hiding this comment.
It seems that this last self.meta.update(kwargs) is (currently) superfluous due to the self.meta.update(self.data_container.meta). Nevertheless, I prefer to keep it as-is, because I would prefer that we, in due time, remove the **kwargs from self.data_container = DataContainer(filename=filename, **kwargs).
Sure, I'll do that. I thought there was something to check before de-drafting this, now I remember 😃 |
|
Notebooks and IRDB tests now pass. The |
Correction: There is one small thing in MICASO_Sci that fails because the interface changed. After talking to @astronomyk , I'll just |
|
Thanks for running the notebooks. I'm fine with some things not working, but we should know about it. (And then indeed mark it as xfail or whatever.) My main fear now is that I'd like to have METIS_Simulations use a newer version of ScopeSim, but that this might break something. If you, @teutoburg, are available to fix such things in the next two weeks, then let's merge this now. Otherwise I don't think this is too urgent so I'm personally also fine with merging this after the METIS_Simulation delivery. I mean, nothing /should/ break, but who knows, experience tells me that often things break anyway. |
Yes, I'm around next week and at ADASS the week after (IIRC you're there as well?). I'm expecting to have enough boredom at the conference to be able to fix things 🙂 |
See the linked issue for the corresponding discussion and rationale...