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

The ragged SPW bonanza #424

Merged
merged 12 commits into from
May 12, 2021
Merged

The ragged SPW bonanza #424

merged 12 commits into from
May 12, 2021

Conversation

o-smirnov
Copy link
Collaborator

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

retest this please

@o-smirnov
Copy link
Collaborator Author

@Athanaseus try this branch to see if it works around (what we assume is) casacore/python-casacore#130

Copy link
Collaborator

@JSKenyon JSKenyon left a comment

Choose a reason for hiding this comment

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

This looks good to me. Can you confirm that you are happy @o-smirnov? I think this will likely need to wait until all the other tiny PRs in limbo are merged, so there may be a couple of conflicts to resolve.

@o-smirnov
Copy link
Collaborator Author

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

@Jordatious
Copy link

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

I'm finding zeros in the data

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

@Jordatious
Copy link

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

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

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

bennahugo commented Mar 17, 2021 via email

@JSKenyon
Copy link
Collaborator

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

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

@JSKenyon
Copy link
Collaborator

Wehey! It worked.

@JSKenyon
Copy link
Collaborator

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.

@JSKenyon JSKenyon merged commit 2822577 into master May 12, 2021
@JSKenyon JSKenyon deleted the issue-93 branch May 12, 2021 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants