Skip to content
Snippets Groups Projects

Conv merge

Merged Jonathan Fisher requested to merge conv-merge into master

Implemented optional convolution filter, fixed various segfaults due to dangling pointers

Merge request reports

Merged by avatar (Jan 2, 2025 11:26pm UTC)

Loading

Pipeline #475 failed

Pipeline failed for d202a21a on master

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • mentioned in issue #12 (closed)

  • mentioned in issue #18 (closed)

  • Thanks for the work. I hardly recognize what I initiated ... By the way, where are the Lorentzian and Gaussian kernels present in the initial convolution_kernel branch ?

  • Those didn't survive the refactoring of the convolver and convolution classes (which were almost totally rewritten), but I can add them back in if there is a desire. The convolver class and the peak finding dialog are implemented in such a way that it is really easy to add new kernels. For biodiff the most important filter to get working was the annular one because it is what they are already using in the HKL2000 software. BTW in the original implementation of the kernels, the kernel was centered incorrectly and produced a shift in the image (the center of the kernel should be at the corners of the image, not the center of the image).

  • other comments:

    • note that blob-finder and peak-finder were refactored to incorporate a ProgressHandler, which can be viewed in the GUI via ProgressView. This allows detailed feedback in the GUI during long calculations. After the merge it would probably be a good idea to incorporate it into other time-consuming calculations (such as indexing or unit cell search)

    • JobHandler.cpp gives simple classes that allow calculations to be run in a separate thread so that the GUI remains responsive. The can be connected to a ProgressHandler/ProgressView so that the user gets feedback about the progress of the calculation.

  • Right, we should propagate your implementation to other time-consuming calculations. BTW, when checking out conv-merge branch and building I get an error when compiling blosc-snappy filter due to unrecognized --std=c++14 flag. However, looking at the CMakeLists.txt files of the project and especially the ones of cblosc, I do not see any deep change there especially for auto-detection of GNU compiler flag. I still work on ubuntu 14.04 with gcc 4.8.4. Would you have any idea ?

    Edited by eric pellegrini
  • I think gcc 4.8.4 may be too old to support c++14. I don't think we are using any c++14 features not present in c++11 so we should probably compile with c++11 instead. Try changing the line in cmake/modules/CheckCompiler.cmake and see if that helps.

  • that does the job. I missed the cmake/modules directory in conv-merge branch.

  • I do not proceed right now to your merge request because when checking peak find on a test D19 data, no peak could be found. I will investigate and possibly come back to you

  • Can you share the data with me? I have only biodiff and D19 data 094542. On 094542 it finds the peaks just as in the old version. If you use the default values in peak find it should behave exactly as in the old version, there is no filter applied unless you manually specify it (and for D19, you have to play with the values a bit to get a good result from the filter)

  • Testing again, there may be a bug where you have to generate a preview before the peak find will work. Let me do a little bit of testing and I will make another commit when I find and fix the problem

  • it is the 094542 that I may have shared with you a while ago. Is that the one you have ?

  • Yes. I did have it working before the merge request but it seems like there might be a bug in the peak find dialog. I will fix it and get back to you

  • Jonathan Fisher Added 1 commit:

    Added 1 commit:

    • 4e4b9477 - bugfix in peak find dialog (crash if no data is viewed in current scene)
  • In current commit, with default values in peak find dialog, I get 62 peaks for D19 dataset 094542. Please confirm whether you get the same result (and whether this is correct!)

    There is another bug, a segfault when the program exits due to some memory being freed twice. This happens only at exit so it is not a fatal bug. I am not sure whether I introduced it, or whether it also exists in master branch. I will try to track it down.

  • Using the master branch and the default values (3.00 for the bkg threshold and 0.997 for the confidence interval), I find 68 peaks that you will find sorted by decreasing intensity in the enclosed file.94542_peaks.txt

  • Jonathan Fisher Added 2 commits:

    Added 2 commits:

    • 5b1fa986 - minor bugfixes related to dangling pointers
    • a73bbaf7 - fixed inconsistent background calculation
  • This is fixed now, I find 68 peaks as before. The problem was that median is treated as a double in new branch instead of integer as before, so we get inconsistent results unless we truncate.

    Before merging, there is still a segfault at program exit which I am trying to track down. It doesn't occur in master to it's a consequence of something in this branch.

  • Jonathan Fisher Added 1 commit:

    Added 1 commit:

    • eea46055 - fixed segfault and memory leak reported by valgrind
  • Please check the latest commit. With default settings I get 68 peaks and the segfault at program exit has been fixed.

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
Please register or sign in to reply
Loading