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

Mitigation for https://github.com/casacore/python-casacore/issues/130 #153

Merged

Conversation

sstansill
Copy link
Collaborator

For further information, see #152.

@sstansill sstansill added the bug Something isn't working label Apr 23, 2024
@sstansill sstansill self-assigned this Apr 23, 2024
@sstansill sstansill linked an issue Apr 23, 2024 that may be closed by this pull request
@CLAassistant
Copy link

CLAassistant commented Apr 23, 2024

CLA assistant check
All committers have signed the CLA.

Copy link
Collaborator

@FedeMPouzols FedeMPouzols left a comment

Choose a reason for hiding this comment

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

Looks Good To Me!

This PR fixes the version of read_col that is used in the conversion function.
I'd guess that getcolslize() is expected to be affected by the same issue as getcol()?
If that's the case, we should (for another issue...) consider if we want to use the workaround also for the "chunked" siblings read_col_chunk() / load_col_chunk() functions that are used as delayed chunk read when using the delayed read MS functions / in the direct load from MS.
The chances that those functions trigger this issue are much smaller though, as
one would not normally go for chunks of several 10s of GBs. That must be the
reason why the issue had not shown up so far in ngCASA experiments (I think) . From the notebook that Sean attached in #152, it seems that the iterator based workaround could actually bring in some performance improvement, but for these two other functions we would need to compare against getcolslice(). I'm also not sure how the iteration by TIME/ANTENNA1/ANTENNA2 would fit.


# Workaround for https://github.com/casacore/python-casacore/issues/130
# WARNING: Assumes tb_tool is a single measurement set not an MMS.
# WARNING: Assumes the num_frequencies * num_polarisations > 2**29. If false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did this mean num_frequencies * num_polarisations < 2**29?

@Jan-Willem Jan-Willem merged commit e5c9bc2 into main May 16, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

casacore getcol() and getcolnp() incorrectly load data
4 participants