Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

542 xarray #543

Merged
merged 234 commits into from
Feb 26, 2024
Merged

542 xarray #543

merged 234 commits into from
Feb 26, 2024

Conversation

Lukas113
Copy link
Collaborator

@Lukas113 Lukas113 commented Feb 14, 2024

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.

lukas.gehrig added 30 commits September 25, 2023 10:56
…fore pushing the image 👠"

This reverts commit a828dfd.
Copy link

codecov bot commented Feb 15, 2024

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 ⚠️
karabo/sourcedetection/result.py 79.31% 6 Missing ⚠️
karabo/imaging/image.py 83.33% 2 Missing ⚠️
karabo/util/file_handler.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            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     

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

Copy link
Collaborator

@lmachadopolettivalle lmachadopolettivalle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 💯

@Lukas113
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

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)

conda/meta.yaml Outdated Show resolved Hide resolved
environment.yaml Outdated Show resolved Hide resolved
karabo/sourcedetection/result.py Show resolved Hide resolved
@Lukas113 Lukas113 marked this pull request as ready for review February 19, 2024 13:56
@Lukas113
Copy link
Collaborator Author

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.

sfiruch
sfiruch previously approved these changes Feb 19, 2024
@kenfus
Copy link
Member

kenfus commented Feb 20, 2024

@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
Copy link
Collaborator Author

@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.

@Lukas113 Lukas113 merged commit d8dcf2e into main Feb 26, 2024
3 checks passed
@Lukas113 Lukas113 deleted the 542_xarray branch February 26, 2024 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants