i4Ds / Karabo-Pipeline

The Karabo Pipeline can be used as Digital Twin for SKA
https://i4ds.github.io/Karabo-Pipeline/
MIT License
11 stars 4 forks source link

Splitting part of source_detection.ipynb off into separate imaging.ipynb example notebook #610

Closed PHerzogFHNW closed 1 month ago

PHerzogFHNW commented 3 months ago

The purpose of this PR is to split off the examples on how to use Karabo for imaging out of the notebook that proves source detection examples, leaving only the WSClean algorithm in the source detection notebook.

This corresponds to https://jira.skatelescope.org/browse/CHOC-18

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 94.11765% with 2 lines in your changes missing coverage. Please review.

Project coverage is 68.78%. Comparing base (45adff6) to head (0848370). Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
karabo/simulation/sample_simulation.py 94.11% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #610 +/- ## ========================================== + Coverage 68.63% 68.78% +0.15% ========================================== Files 53 54 +1 Lines 5714 5748 +34 ========================================== + Hits 3922 3954 +32 - Misses 1792 1794 +2 ``` | [Flag](https://app.codecov.io/gh/i4Ds/Karabo-Pipeline/pull/610/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=i4Ds) | Coverage Δ | | |---|---|---| | [](https://app.codecov.io/gh/i4Ds/Karabo-Pipeline/pull/610/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=i4Ds) | `68.78% <94.11%> (+0.15%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=i4Ds#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Lukas113 commented 3 months ago

In addition, writing the down the purpose of this PR would be helpful rather than pointing to a JIRA ticket where permissions are needed to access.

PHerzogFHNW commented 3 months ago

I added the corresponding test and a small description in the original comment, as those are no-brainers. I copied that format off someone who must've been equally lazy, my bad.

As for the rest, it might be best to wait until Michel is back since I did this implementation as per his instructions. I do agree that the duplication is bad for the reasons you listed. Discussing this with Michel, we could not come up with a better solution as doing the initial simulation would be part of the expected workflow of a scientist doing either imaging or source detection.

Lukas113 commented 3 months ago

Sure we can wait. I also discussed this matter with Mathias Graf. And we concluded more or less to the solution I proposed. And since the changes are small (at least if I considered every aspect of it which might not be the case), it's hard to see for me why we shouldn't do the suggested approach.

mpluess commented 2 months ago

The idea was for this PR to be just about splitting up source detection and imaging, but since the topic of simulation code duplication came up multiple times during the last weeks (e.g. Lukas' metadata example script IIRC), it might indeed be a good idea to address it right now. I could imagine a slightly simpler solution than Lukas suggested: what about just having a saved FITS file that lives in the repo and replace the simulation code with a simple loading of that file? That would save us env vars, cleanup, ordered tests. I believe in the SRCNet_rucio_meta.py script, we also need telescope and observation objects, but we could just load them from disk as well using serialization. Opinions?

sfiruch commented 2 months ago

[...] I believe in the SRCNet_rucio_meta.py script, we also need telescope and observation objects, but we could just load them from disk as well using serialization. Opinions?

Depends on the size. It also means that we'd have to update serialized files whenever internals change.

Why don't we add production of example-data to the Karabo API? A function à la get visibilities for tests, experiments and examples() could be useful?

mpluess commented 2 months ago

[...] I believe in the SRCNet_rucio_meta.py script, we also need telescope and observation objects, but we could just load them from disk as well using serialization. Opinions?

Depends on the size. It also means that we'd have to update serialized files whenever internals change.

Why don't we add production of example-data to the Karabo API? A function à la get visibilities for tests, experiments and examples() could be useful?

Sounds like a good solution. Can be a small simulation so it doesn't take much time when run repeatedly in different parts of the code base.

Lukas113 commented 2 months ago

Not sure... because to me it seems like the proposed solution is way easier to implement and doesn't require any dedicated file generation function, just a get_vis_path_from_* function which is looking at an env-var (or even a hard-coded path).

However, since you both don't agree I'm not really sure what to say other that I disagree. What matters to me in the end is to have minimal code duplication for maintainability & example clarity.

mpluess commented 1 month ago

@Lukas113 merging is blocked because of your change request in august, do you want to have another look or alternatively withdraw the change request?