search

mtazzari / galario

Gpu Accelerated Library for Analysing Radio Interferometer Observations
https://mtazzari.github.io/galario/
GNU Lesser General Public License v3.0
31 stars 15 forks source link

[bugfix] central pixel interpolation #133

Closed mtazzari closed 6 years ago

mtazzari commented 6 years ago

Still to be tested on GPU

mtazzari commented 6 years ago

Now tested on GPU, all tests pass.

mtazzari commented 6 years ago

TODO: Add a warning if dR>dxy/5. It is recommended to have at least 10 points in the central pixel. @fredRos where should we add this warning? It should appear when using *Profile() funcs and sweep.

mtazzari commented 6 years ago

I get Travis errors when compiling the cpp. Is it possible you solved this in the other PRs?

The following tests FAILED:
      2 - cpp_compile_test (SEGFAULT)

Fixed in 102a30dff2385405ae97c7937adf2dc8188f1e51 It was caused by random numbers used in the cpp test. Now I use some sensible numbers for nx, dR, Rmin and it works fine.

fredRos commented 6 years ago

TODO: Add a warning if dR>dxy/5. It is recommended to have at least 10 points in the central pixel. @fredRos where should we add this warning? It should appear when using *Profile() funcs and sweep

What do you mean by points? Interpolation points? You can do that check in the code but can you also just check dR>dxy/5? Since you have these numbers at the C++ level, let's do the check and the warning (to std::cerr there. Always prefer to have these checks at the lowest level at which you have the necessary information

fredRos commented 6 years ago

The tests pass on the GPU. Please rebase onto master, fix any conflicts and force push again to this branch

mtazzari commented 6 years ago

If I use the web editor to solve conflicts is it ok? Is the same as rebasing?

mtazzari commented 6 years ago

I solved all the conflicts. I don't understand why github says that This branch cannot be rebased due to conflicts @fredRos does this mean that I have to merge the branch via command line, e.g. with:

git checkout master
git merge --no-ff central_pixel_bugfix
git push origin master

?

mtazzari commented 6 years ago

All tests pass on GPU as well.

mtazzari commented 6 years ago

A clarification: I understand that if a PR has conflicts to be fixed via command line, then Rebase and merge option will not be possible, but a merge commit is necessary. Is this correct?

fredRos commented 6 years ago

A clarification: I understand that if a PR has conflicts to be fixed via command line, then Rebase and merge option will not be possible, but a merge commit is necessary. Is this correct?

No, you can always avoid merge commits with this strategy:

git checkout central_pixel_bugfix # assuming this branch is up to date
git rebase origin/master
git push -f origin
mtazzari commented 6 years ago

All CPU/GPU tests pass. Ready to be merged.