Maclariz ddf patch 1

[maclariz]

Here are my functions for Digital Dark Field Imaging. The one thing you might want to change is what progress bar you use to make it consistent with things you use elsewhere. I just used the progress bar library I have been using for a while now. Other than that, it's all just numpy and base python and py4dstem.

[maclariz]

Some minor changes to variable naming in one of the functions, and improvement of the inline comments to make them fully consistent with the function variable names

[maclariz]

I presume reading the errors that it was just import numpy as np that was killing things. I have still used an imported progress bar function as I don't understand yours and don't see any code definition for this anywhere to call. You may wish to advise me about this before running another attempt to merge.

[gvarnavi]

Thanks for the contribution!

Approved the automatic workflow runs, linted the code with black, and switched the progress bar to the internal tqdmnd (the documentation of which can be found here). You might want to check the code still runs as you expect it to, since I did no checking.

I'll leave the reviewing for others who work with virtual imaging more, but here's some quick comments:

• The code is now not imported in any namespaces. This can be achieved by adding a __init__.py file in the digitaldarkfield directory, and an appropriate line in the process directory __init__.py. You can see an example here
• More generally, I suspect a conversation on where this would best fit is warranted. This is now implemented as functions and not its own class. This is fine, but perhaps then it's appropriate to expose these functions as a DataCube method? See e.g. how virtual imaging is implemented
• We're trying to switch to using docstrings to document functions, with a style similar to the numpy docs one - you can see an example here

[maclariz]

@gvarnavi I don't understand what the init.py is doing. I am not a great programmer and certainly not well up on what the conventions are being used by everyone. And there is little in the way of suitable training at my university to help with that. What I am good at is problem solving and finding mathematically and computationally efficient ways to do stuff that is physically meaningful (i.e. doing physics in a reasonably short amount of time). Quite happy to be educated as to how to make things better fit the larger ecosystem.

[maclariz]

@gvarnavi @cophus the wider conversation on fit is welcomed!

I don't know enough about classes yet.

It is not a Datacube method, per se. It is a pointslists method...

[maclariz]

@gvarnavi changing to your docsstrings comments is easy. Will test the approved version, and provided that is now okay, I will make another fork and then edit and make a pull request.

[gvarnavi]

@maclariz We're very happy to help with adding __init__.py files / importing the module according to our conventions!

Perhaps the easiest thing to do is for @cophus (or similar) to review the code for scientific content and once that is sorted, and we've decided on where the best fit is, I'm happy to help with massaging the code structure to fit our conventions.

Re: your comment on making a new fork/pull-request: You can simply keep working on your maclariz-DDF-patch-1 branch, and any commits you push there will be added to this pull request.

[maclariz]

@gvarnavi I have now requested that our sysadmin (not me, but someone far more suitable) makes a new venv with a suitable python version for me to run my own fork of py4dstem on for testing. I'll test any further updates in there.

[cophus]

Testing notebook using the new DDF syntax:

https://drive.google.com/file/d/1WA7DAjBEYpIOk24T7y4NqRPvJj5_Kfpg/view?usp=sharing

[smribet]

Hi Ian and Colin,

Thank you for this PR! I think this will be very helpful for a number of experiments. I am happy to merge but have two small requested changes:

1. Can you please add doc strings? They are missing for a few functions. aperture_array_generator has docstrings twice (although I do appreciate the extra notes about mode).
2. bplist.cal will apply all calibrations attached to the braggpeaks, which will create problems if the pixel size of the braggpeaks is calibrated (or it inherits a calibration from a datacube). I would suggest instead of using bplist.cal to follow the design pattern in ACOM and pass those options to the user as flags.

~Steph

[maclariz]

Hi Steph, I have tried updating the docstrings to standard format and being explicit about everything. Some were updated by Colin after he tweaked things. I know there is some repetition for aperture_array_generator but the extra information up top about slight tweaks in how parameters are used in different modes is useful. I don’t think calibrated pixel sizes causes any issues, as long as calibration is also used in the aperture_array. As noted at the top, tolerance setting will be totally different if we are in calibrated units. That’s pretty much it. The rest is just matching lists of numbers, and the absolute magnitude is irrelevant as long as everything is in the same units.

Best wishes

Ian

On 28 Aug 2024, at 16:29, Stephanie Ribet @.***> wrote:

Hi Ian and Colin,

Thank you for this PR! I think this will be very helpful for a number of experiments. I am happy to merge but have two small requested changes:

1. Can you please add doc strings? They are missing for a few functions. aperture_array_generator has docstrings twice (although I do appreciate the extra notes about mode).
2. bplist.cal will apply all calibrations attached to the braggpeaks, which will create problems if the pixel size of the braggpeaks is calibrated (or it inherits a calibration from a datacube). I would suggest instead of using bplist.cal to follow the design pattern in ACOMhttps://github.com/py4dstem/py4DSTEM/blob/2d8ce83cb7beadb1d101fcf0178782e1e4bff5e4/py4DSTEM/process/diffraction/crystal_ACOM.py#L925C9-L932C10 and pass those options to the user as flags.

~Steph

— Reply to this email directly, view it on GitHubhttps://github.com/py4dstem/py4DSTEM/pull/665#issuecomment-2315676445, or unsubscribehttps://github.com/notifications/unsubscribe-auth/APHUIKULQUMRTJKBW6WDW2TZTXUD7AVCNFSM6AAAAABJ4TPA5CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMJVGY3TMNBUGU. You are receiving this because you were mentioned.Message ID: @.***>

Dr Ian MacLaren (he, him, his) BSc (Hons), PhD, FInstP, CPhys Reader in Physics

Materials and Condensed Matter Physics School of Physics and Astronomy / Sgoil Fhiosaigs is Reul-Eòlais University of Glasgow / Oilthigh Ghlaschu Glasgow G12 8QQ

http://www.gla.ac.uk/schools/physics/research/groups/mcmp/

https://publons.com/researcher/C-1773-2010/ ORCID: 0000-0002-5334-3010

The University of Glasgow, charity number SC004401

[smribet]

Hi Ian,

Thanks for making these changes! I agree that all the information in the doc strings is helpful!

As you point out, it would be straightforward to account for pixel sizes. Unfortunately, braggvector calibrations are especially confusing, so I tweaked the code to make this a bit more explicit and to give a user more control. The default behavior should match what you had. Let me know if this looks ok to you, and I can merge!

~Steph