Skip to content

Conversation

jakubwro
Copy link
Collaborator

Hi, please review. If you think it's worth merging I'll update docs.

@giordano
Copy link
Member

Wow, this is great, thank you very much! I'll review later.

This PR also reminds me that I should move documentation to Documenter 😓

@giordano giordano self-requested a review November 11, 2019 09:36
Copy link
Member

@giordano giordano left a comment

Choose a reason for hiding this comment

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

I cannot comment on the algorithm in particular as I'm not familiar with it, but the code looks good, thank you so much!

If you think it's worth merging I'll update docs.

Yes, please 🙂

Co-Authored-By: Mosè Giordano <giordano@users.noreply.github.com>
@codecov-io
Copy link

codecov-io commented Nov 13, 2019

Codecov Report

Merging #8 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master     #8   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           2      3    +1     
  Lines          16     25    +9     
=====================================
+ Hits           16     25    +9
Impacted Files Coverage Δ
src/Deconvolution.jl 100% <ø> (ø) ⬆️
src/lucy.jl 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4cb5227...77977db. Read the comment docs.

@jakubwro
Copy link
Collaborator Author

jakubwro commented Dec 1, 2019

Hi @giordano , I added docs and more illustrative usage example

@giordano
Copy link
Member

giordano commented Dec 1, 2019

Awesome, I'll have a look in the next few days!

@jakubwro
Copy link
Collaborator Author

jakubwro commented Dec 6, 2019

@giordano, I readded docs, but I have a feeling that messed something with git #history. Please have a look.

@giordano
Copy link
Member

giordano commented Dec 6, 2019

The history is not a problem, we can just squash everything 😉

I followed your struggle, I'm really sorry for this. I can feel your pain, I had similar problems a few days ago. In theses cases I think that it's easier to do a clean merge of the target branch on yours rather than rebasing.

*.jl.*.cov
*.jl.mem
/doc/_build/
/doc/build/
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@abhishalya , Documenter uses different output folder and .gitignore should be changed. Sorry for not catching that during review.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, right. Sorry I missed that too. Thanks for correcting that :)

@jakubwro
Copy link
Collaborator Author

jakubwro commented Dec 6, 2019

The history is not a problem, we can just squash everything wink

I followed your struggle, I'm really sorry for this. I can feel your pain, I had similar problems a few days ago. In theses cases I think that it's easier to do a clean merge of the target branch on yours rather than rebasing.

Right, it's not the first time I have this problem with rebase.

@giordano giordano closed this Dec 6, 2019
@giordano giordano reopened this Dec 6, 2019
@giordano giordano merged commit 655f59f into JuliaDSP:master Dec 6, 2019
@giordano
Copy link
Member

giordano commented Dec 6, 2019

Documentation is available at https://juliadsp.github.io/Deconvolution.jl/dev/. Thank you so much @jakubwro and @abhishalya! This new method looks very good!

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.

4 participants