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

542 xarray #543

Closed Lukas113 closed 7 months ago

Lukas113 commented 7 months ago

See issue #542

Additional bug fixing and minor improvements

Unfortunately, I created this branch from 512_sarus and not from main, if you wonder why there are so many commits.

codecov[bot] commented 7 months ago

Codecov Report

Attention: Patch coverage is 77.52809% with 20 lines in your changes are missing coverage. Please review.

Project coverage is 65.85%. Comparing base (e1b75fa) to head (5780c32).

Files Patch % Lines
karabo/imaging/imager.py 73.17% 11 Missing :warning:
karabo/sourcedetection/result.py 79.31% 6 Missing :warning:
karabo/imaging/image.py 83.33% 2 Missing :warning:
karabo/util/file_handler.py 0.00% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #543 +/- ## ========================================== + Coverage 64.75% 65.85% +1.09% ========================================== Files 48 48 Lines 4687 4750 +63 ========================================== + Hits 3035 3128 +93 + Misses 1652 1622 -30 ```

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

Lukas113 commented 7 months ago

@kenfus is it okay to remove guess_beam_parameters in this PR? I removed it because it never worked. If you have a good reason to keep it and ensure that it works, I'll happily add the function to the module again.

Lukas113 commented 7 months ago

An update, with pandas >=2, the following FutureWarning occured:

/home/lukas/miniconda3/envs/karabo_dev_env/lib/python3.9/site-packages/ska_sdp_datamodels/visibility/vis_model.py:74: FutureWarning: the `pandas.MultiIndex` object(s) passed as 'baselines' coordinate(s) or data variable(s) will no longer be implicitly promoted and wrapped into multiple indexed coordinates in the future (i.e., one coordinate for each multi-index level + one dimension coordinate). If you want to keep this behavior, you need to first wrap it explicitly using `mindex_coords = xarray.Coordinates.from_pandas_multiindex(mindex_obj, 'dim')` and pass it as coordinates, e.g., `xarray.Dataset(coords=mindex_coords)`, `dataset.assign_coords(mindex_coords)` or `dataarray.assign_coords(mindex_coords)`.
  super().__init__(data_vars, coords=coords, attrs=attrs)
Lukas113 commented 7 months ago

I also removed the build-string fixations, because they prevent updates on dependency-constraints. Therefore, I introduces the following instructions to the README of the Feedstock & triggered the according builds there.

Updating an already existing Wheel: It may happen that an update of a dependency (direct or transversal) introduces a breaking change in a future release. Then, the current constraints of an existing conda-wheel would be outdated an will most likely result in a broken environment. To prevent this, updating the according meta.yaml and increase the build-nr is not enough, because the solver doesn't just consider the largest build-numbers of conda-wheels. So, to fix this for current and also older releases, you have to relabel oudated conda-wheels to legacy. Also do a comment in the meta.yaml, why you did constrain the dependency. This makes future builds where the constraints may have to be reconsidered way easier. It was already mentioned in this paragraph, but don't forget to increase the build-number.

kenfus commented 7 months ago

@kenfus is it okay to remove guess_beam_parameters in this PR? I removed it because it never worked. If you have a good reason to keep it and ensure that it works, I'll happily add the function to the module again.

I rather not. This was requested by @rohitcbscient and it also works in the current example of the source_detection.ipynb.

Lukas113 commented 7 months ago

@kenfus is it okay to remove guess_beam_parameters in this PR? I removed it because it never worked. If you have a good reason to keep it and ensure that it works, I'll happily add the function to the module again.

I rather not. This was requested by @rohitcbscient and it also works in the current example of the source_detection.ipynb.

Okok. Not sure what failed though, because after everything has settled, it works for me as well. However, I still improved the function by not changing the imager-fields permanently just because of a guess-function. Update will soon be available.