search

ratt-ru / CubiCal

A fast radio interferometric calibration suite.
GNU General Public License v2.0
18 stars 13 forks source link

The ragged SPW bonanza #424

Closed o-smirnov closed 3 years ago

o-smirnov commented 3 years ago

Still some testing and verification to be done, but I wanted @JSKenyon's review to start at his earliest convenience (this being the holidays, this could understandably be delayed...)

o-smirnov commented 3 years ago

retest this please

o-smirnov commented 3 years ago

@Athanaseus try this branch to see if it works around (what we assume is) https://github.com/casacore/python-casacore/issues/130

o-smirnov commented 3 years ago

I'm happy. @Jordatious, does this branch solve your ragged-SPW issues?

Jordatious commented 3 years ago

It's hard to say. I'm just finishing inspecting the results (hence this issue) and then will report on it. It at least ran end-to-end, and peeled the source, but with a lot of junk left over. I'm finding zeros in the data for some reason, so I think perhaps something was funky from before CubiCal ran.

IanHeywood commented 3 years ago

I'm finding zeros in the data

Off-topic, but were there zeros in the data as well as the model?

Jordatious commented 3 years ago

Hard to say from the plots here (data and corrected). Also not sure about DIR1_DATA, only MODEL_DATA at this point. I'll check. image image

JSKenyon commented 3 years ago

@o-smirnov tests seem to be failing with:

Traceback (most recent call last):
  File "/usr/local/bin/gocubical", line 5, in <module>
    from cubical.main import main
  File "/usr/local/lib/python3.6/dist-packages/cubical/main.py", line 102, in <module>
    from cubical.data_handler.ms_data_handler import MSDataHandler
  File "/usr/local/lib/python3.6/dist-packages/cubical/data_handler/ms_data_handler.py", line 0
SyntaxError: unknown encoding: future_fstrings

I suspect that future_fstrings could be added to setup.py, but I actually think that this is unnecessary. As we are dropping support for Python<3.6 (and soon we may need to consider dropping support for 3.6), there is no need for the backporting. Could you please remove the encodings?

o-smirnov commented 3 years ago

I thought it was in setup.py already? We've been using f-strings all over for quite a while.

I would add it to setup.py and leave it like this for one more release cycle. Once we're fully settled on >3.6, we can sanitize the unnecessary cookies.

bennahugo commented 3 years ago

Please do not drop python3.6 support until we have not switched our production systems to 20.04. I also plan to only support up to 3.6 for the next release of kms and ddfacet so let's keep synergy between the tools.

On Wed, 17 Mar 2021, 10:41 JSKenyon, @.***> wrote:

@o-smirnov https://github.com/o-smirnov tests seem to be failing with:

Traceback (most recent call last): File "/usr/local/bin/gocubical", line 5, in from cubical.main import main File "/usr/local/lib/python3.6/dist-packages/cubical/main.py", line 102, in from cubical.data_handler.ms_data_handler import MSDataHandler File "/usr/local/lib/python3.6/dist-packages/cubical/data_handler/ms_data_handler.py", line 0 SyntaxError: unknown encoding: future_fstrings

I suspect that future_fstrings could be added to setup.py, but I actually think that this is unnecessary. As we are dropping support for Python<3.6 (and soon we may need to consider dropping support for 3.6), there is no need for the backporting. Could you please remove the encodings?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/ratt-ru/CubiCal/pull/424#issuecomment-800903416, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB4RE6QGO5DPTKHHJC42TR3TEBTM7ANCNFSM4VBQTC5Q .

JSKenyon commented 3 years ago

I have not done anything to 3.6 functionality, I was just saying that we will likely be forced to drop it in the near future due to upstream pressure. future-fstrings is only necessary for <3.6. Given that those versions are not supported, we don't need it. I have removed the two -*- coding: future_fstrings -*- that crept in with this PR and will see if the tests run through.

o-smirnov commented 3 years ago

Ah. Well if it's only <=3.5 that's being affected, then let it burn indeed!

JSKenyon commented 3 years ago

Wehey! It worked.

JSKenyon commented 3 years ago

I will leave pushing the button to you @o-smirnov, once you are happy that the issues @Jordatious is still seeing are not in some way related to these changes.