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

frelon_peaksearch process function split into two functions #395

Closed
wants to merge 4 commits into from

Conversation

abmajith
Copy link
Contributor

@abmajith abmajith commented Feb 7, 2025

Hello @loichuder, @jadball

I split the frelon_peak search process function into two, first for running the segment on frames in parallel,
and then merging the peaks found on each frames, so on.

@@ -360,6 +360,95 @@ def guess_bg(ds, scan_number=0, start=0, step=9, n=None):
PKCOL = [getattr(ImageD11.cImageD11, p) for p in PKSAVE]


def collect_all_frames_peaks(
dataset,
Copy link
Contributor

@loichuder loichuder Feb 7, 2025

Choose a reason for hiding this comment

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

Perhaps a bit early but as @jadball suggested, we should not need the Dataset class in the future.

We can give the masterfile, frames_dataset and omega_angles as arguments of the function so that we can reuse it without further changes when we remove the use of the Dataset class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct, this will make future compatible!

return peaks_2d_dict, num_peaks


def process_dataset_for_peaks_columnfile(
Copy link
Contributor

Choose a reason for hiding this comment

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

This is essentialy a reimplementation of the process function below right ?

Should we use the new functions directly in the process function instead of creating a new process function ? What do you think @jadball ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, that is true,

Copy link
Member

@jonwright jonwright left a comment

Choose a reason for hiding this comment

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

Maybe I missed some discussion prior to this? The 'importScan.py' code looks like an extraction/refactoring of dataset.py but it is not imported or used in frelon_peaksearch.py. Is this intended to be part of this PR?

Your type hints for CI seem to require https://peps.python.org/pep-0484/#suggested-syntax-for-python-2-7-and-straddling-code

@abmajith abmajith closed this Feb 7, 2025
@abmajith
Copy link
Contributor Author

abmajith commented Feb 7, 2025

Maybe I missed some discussion prior to this? The 'importScan.py' code looks like an extraction/refactoring of dataset.py but it is not imported or used in frelon_peaksearch.py. Is this intended to be part of this PR?

I wrote this Import_scan to do the segmentation with minimal dataset class method but subset of dataset class method, to make the columnfile treatable from its own class, instead of doing it in dataset class

I will remote it now, and once I test it completely I will ask for merge in future, sorry of the confusion I made

@jonwright
Copy link
Member

No problem - if I were you, I would be tempted to run the segmentation starting just from the bliss "master.h5 + scan" and see if you can drop everything else from our dataset class. It was intended for assembling 2D sinograms from many 1D scans...

@loichuder
Copy link
Contributor

The 'importScan.py' code looks like an extraction/refactoring of dataset.py but it is not imported or used in frelon_peaksearch.py. Is this intended to be part of this PR?

Yes. I don't think it was intended for this PR.

@abmajith As discussed this week, you don't have to close the PR: keep 633012d only, which the commit that we discussed, in this branch.

You could then keep the other commits in another branch in the meantime.

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.

3 participants