Be lens calc follow up #8

cristinasewell · 2020-10-21T15:58:12Z

The lens_set file has been changed from using numpy to using json to save and read the data from the file.
The plan_set function has been added
A new lens_transmission function has been added too

Motivation and Context

How Has This Been Tested?

It is hard to test this, tested against the old function and seems to work the same, but more testing needs to be done....

Where Has This Been Documented?

Docstring

cristinasewell · 2020-10-21T16:48:20Z

pcdscalc/be_lens_calcs.py

+    logger.info('\n %s', resstring)
+
+
+def lens_transmission(r, fwhm, n=1, energy=None, id="Be", lens_thicknes=None):


In the old code we have ID='IF1' - is the ID here the material? that's what is seems like but IF1 is not a valid material for the current get_att_len function
https://github.com/pcdshub/old-hutch-configs/blob/18c7769d66a647ab558ad7ebc34d019fc98fbf48/cxi/blutil/calc.py#L1189

Small horror story here: the old hutch python actually used a modified version of periodictable. So there very well could have been an added entry named IF1.

Do you think we'll need to support it?

I hope not. I'll quickly go looking for this illicit version to see what it does.

Ok, a grep for IF1 is turning up empty

Actually I just found it here, in the same file you linked: https://github.com/pcdshub/old-hutch-configs/blob/18c7769d66a647ab558ad7ebc34d019fc98fbf48/cxi/blutil/calc.py#L1815

ohh noo 😭

Ok, I am going to add the aliases here then for the chemical formulas that they have. But in order to make it work with xrayDB I have to provide the density too, so I'll have to bring in the Density as well...

added this in - 1fcd6fb - but I don't like it, should we have those densities and material in a separate file?

Keeping them in code is fine for now, in my opinion. Then yaml or JSON down the line.

cristinasewell · 2020-10-21T16:52:15Z

pcdscalc/tests/test_be_lens_calcs.py

+
+
+# with good_sets = [False, True, True, False, False, True, False, False, False]
+def test_plan_set():


It is hard to make the plan_set generate some True for good_sets to be able to test the results.... I've been testing it with dummy good_sets against the old function and I've been getting the similar results, but obviously I have to find some good parameters to test this

Seems good enough to me. The values provided to that function don't seem to change the control flow, meaning testing arbitrary-ish values against the old implementation is likely sufficient.

I agree with Ken, this is good

pcdscalc/be_lens_calcs.py

cristinasewell · 2020-10-21T20:06:56Z

pcdscalc/be_lens_calcs.py

+    lens_thicknes = lens_thicknes or APEX_DISTANCE
+    id_material = check_id(id_material)
+    # TODO: what should I do with the energy here
+    # energy = getE(energy=energy, correctEv=True)


this function requires a PV, should we have that in the calculations or is this up to to user to pass in the correct energy when using the classes?
https://github.com/pcdshub/old-hutch-configs/blob/18c7769d66a647ab558ad7ebc34d019fc98fbf48/cxi/blutil/calc.py#L10-L40

There shouldn't be anything that requires a PV in this codebase - values only.

The previous codebase checked a PV if you didn't provide an energy value. I'm not sure exactly how/where to implement this behavior... perhaps not until we get to pcdsdevices and you need to move to a specific beam size?

Generally when you're planning an experiment the current x-ray energy does not match what it will be during your beam time.

@ZLLentz what's the issue in requiring the user to be explicit about the energy as an input to these functions? Or rather, why do you think that functionality from the old codebase need to be included here?

When you call these functions you should call them with the energy you expect to have during the beam time. There may be some cases during a beam time when you want to call a function with "current beam energy". These are mostly localized around "I want to move my calculation motor without repeatedly updating the beam energy parameter"

I see - perhaps our LCLS object should have an easily toggleable subscription that calls the new equivalent of setE for setting the default energy when none is supplied by kwarg?

@klauer when you get a chance can you please elaborate on this^?

Will it look similar to this: (in the LCSL object)

photon_ev_hxr = Cpt(EpicsSignalRO, 'SIOC:SYS0:ML00:AO627', kind='hinted', doc='Photon eV HXR [eV]') @photon_ev_hxr.sub_value def _energy_changed(self, *args, value, ** kwargs): self.set_energy(value) print(f'----VALUE CHANGED -------{value}') def set_energy(self, value): global _photon_energy _photon_energy = value def get_energy(self): return _photon_energy

And then in the LensStack object I would get this value from the LCLS object?

lcls = LCLS() lcls.get_energy()

I was thinking something along the lines of:

photon_ev_hxr.subscribe(_set_current_energy) def get_energy(): if user_specified_energy is not None: return user_specified_energy return _current_energy def other_calculation_function(, ..., energy=None): if energy is None: energy = get_energy()

It's a lot of customization of one parameter - but it's apparently an important one to be flexible with here.

I'm still a bit confused here - i'm so sorry.
Here is what i understand:
The photon_ev_hxr.subscribe(_set_current_energy) is in the beam_stats LCLS object. Here is where I will have access to the current_energy - but I don't need to know here about the user_specified_energy - or do i? - i'm not doing any calculations here.
I would need to know about the user specified energy in the LensStack and in the calculations...

I need to think about this a bit more, i might be overcomplicating things here

The user_specified_energy is the user-provided energy that acts as an override for the current energy provided by the LCLS subscription callback.

Maybe I'm missing something here, too - not sure where the confusion is coming in. (@ZLLentz?)

I like (3) - let's get this rolling and in the hands of those that can use it 👍

Sounds good, at least if people start using it we'll get some feedback (maybe) from them and hopefully some of the questions/concerns will be answered...

klauer · 2020-10-21T18:41:05Z

pcdscalc/be_lens_calcs.py

+    >>> list_of_sets = [[3, 0.0001, 1, 0.0002],
+                        [1, 0.0001, 1, 0.0003, 1, 0.0005],
+                        [2, 0.0001, 1, 0.0005]]
+    >>> set_lens_set_to_file(list_of_sets, ../path/to/lens_set)


Suggested change

>>> set_lens_set_to_file(list_of_sets, ../path/to/lens_set)

>>> set_lens_set_to_file(list_of_sets, '../path/to/lens_set')

klauer · 2020-10-21T22:35:15Z

pcdscalc/be_lens_calcs.py

-        if set_number_top_to_bot not in range(1, sets.shape[0]):
+    with open(filename) as lens_file:
+        sets = json.loads(lens_file.read())
+        if set_number_top_to_bot not in range(1, len(sets)):


This and other errors in the function should probably be bumped up to exceptions (ValueError) as they'd break the calling function by returning None

klauer · 2020-10-21T22:38:09Z

pcdscalc/be_lens_calcs.py

+    logger.info('\n %s', resstring)
+
+
+def lens_transmission(r, fwhm, n=1, energy=None, id="Be", lens_thicknes=None):


Keeping them in code is fine for now, in my opinion. Then yaml or JSON down the line.

klauer · 2020-10-21T22:42:49Z

pcdscalc/tests/test_be_lens_calcs.py

+
+
+# with good_sets = [False, True, True, False, False, True, False, False, False]
+def test_plan_set():


Seems good enough to me. The values provided to that function don't seem to change the control flow, meaning testing arbitrary-ish values against the old implementation is likely sufficient.

cristinasewell · 2020-10-22T06:22:53Z

pcdscalc/be_lens_calcs.py

+            )
+            thickness[i, j] = (x[i] ** 2 + y[j] ** 2) / r + n * lens_thicknes
+    d = density.get(id_material)
+    att_length = get_att_len(energy, id_material, d)


Need to revise this one, I'm using the get_att_len here (and tested it with get_att_len in the old code too), but after doing some research I realized that this is not the same as attenuationLength function from https://github.com/pcdshub/old-hutch-configs/blob/18c7769d66a647ab558ad7ebc34d019fc98fbf48/xpp/prod/utilitiesCalc.py#L362
which brings with it a few more other functions.... that require periodictable...

…tdout - maybe later we'll change them to a tabulated form or something

pcdscalc/be_lens_calcs.py

ZLLentz · 2020-10-26T21:18:49Z

pcdscalc/be_lens_calcs.py

+
+    Parameters
+    ----------
+    energy : number


Most of the input parameters here need to have units associated with them. It's probably good to check other functions as well so that we don't get confused later.

Ok, i added to some of these functions in 279f107 but not to all of them (because I am not 100 % sure)
I am almost sure that the radius that we're using from the lens_set and in most of the functions are in meters, so i can add that too but I am not 100 % convinced.

Ok we need to figure these out then, if the wrong units are used then the calcs will not work as expected

Yeah I agree - i'm going to go back and try to figure out all of them. Based on the old curren_set file we have these:

[(3, 0.0001, 1, 0.0002), (1, 0.0001, 1, 0.0003, 1, 0.0005), (2, 0.0001, 1, 0.0005)]

So it make sense that they would be in meters, because we have the LENS_RADII = [50e-6, 100e-6, 200e-6, 300e-6, 500e-6, 1000e-6, 1500e-6, 2000e-6, 3000e-6] which are in um
I need to double check here though ...

pcdscalc/be_lens_calcs.py

ZLLentz · 2020-10-28T17:27:28Z

pcdscalc/be_lens_calcs.py

+
+
+def plan_set(energy, z_offset, z_range, beam_size_unfocused, size_horizontal,
+             size_vertical=None, exclude=[], max_tot_number_of_lenses=25,


It doesn't matter here, but sometimes you can run into trouble if you pass a mutable object like a list as the default argument to a function. This is because if you modify this list in the function call, the modified list will be used as the default value in the next call of the function, rather than the original list.

Ok, thank you for pointing this out. I can make it None and inside the function do something like this:
exclude = exclude or []?

Yes, that is the typical way to handle this, using an immutable sentinel like None

ZLLentz · 2020-10-28T17:28:11Z

pcdscalc/be_lens_calcs.py

@@ -42,7 +45,7 @@ def configure_defaults(fwhm_unfocused=None, disk_thickness=None,
    Parameters
    -----------
    fwhm_unfocused : float, optional
-        Full width at half maximum unfocused
+        Full width at half maximum unfocused.


Suggestion to match the unit in the other docstrings

Suggested change

Full width at half maximum unfocused.

Full width at half maximum unfocused in meters.

ZLLentz · 2020-10-28T17:30:05Z

pcdscalc/be_lens_calcs.py

    size_fwhm : float
    distance : float
    n_max : int, optional
    max_each : int, optional
    lens_radii : list, optional
    fwhm_unfocused : float, optional
-        This is about 400 microns at XPP. Defaults to 500e-6.
+        This is about 400 microns at XPP. Defaults to 500e-6. (in meters)
    eff_rad0 : float, optional


I think this docstring is missing some descriptions too, though they are the same parameters as other functions in the file

I am not sure here what eff_rad0 is - thinking that could be Effective Radius of curvature? but not sure
any ideas?

Also I am not sure of these guys' units:

# Disk Thickness DISK_THICKNESS = 1.0e-3 # Apex of the lens APEX_DISTANCE = 30e-6

again i am assuming that they have to be meters - will come back to this one in a sec

1mm and 30µm sound like reasonable values for those, so I'd guess meters is correct.

ZLLentz · 2020-10-28T17:44:14Z

I want to say that this is really close

cristinasewell · 2020-10-28T20:00:33Z

I still don't feel comfortable for the Lens Transmission in the plan_set function, but I think I will know better when Matt gets back to me. I think we could be using the calc_trans_lens_set or calc_trans_for_single_lens - but we can adjust this later when we have more info..?

ZLLentz · 2020-10-28T22:13:32Z

Yeah, we have a few options:

Be patient for a response from the experts
Merge, but don't tag, and fix discrepancies in a follow-up PR
Merge, tag, and start including the functions in hutch-python/pcdsdevices as necessary, with the knowledge we may need to revise the base calculations later.

I'm leaning toward merging- otherwise this continues to get more difficult to review as it grows in scope.

klauer · 2020-10-28T23:50:17Z

I like (3) - let's get this rolling and in the hands of those that can use it 👍

cristinasewell · 2020-10-30T06:39:41Z

I thought I added a comment earlier today here but I don't see it - I don't know what happen to it.
Anyway, I agree with you about option 3 - when people will start using these calculations we'll probably get feedback and then we can adjust things if needed.

ZLLentz · 2020-11-02T19:10:56Z

pcdscalc/be_lens_calcs.py


 import numpy as np
 import xraydb as xdb

 logger = logging.getLogger(__name__)
+# global variable to be help with testing the plan_set function
+_plan_set_test_res = None


I think this was a good solution and doesn't need a revision. Another way to do this is to have testable helper functions with whatever return values we want, and use the helpers in the functions without good return values.

ZLLentz · 2020-11-02T19:23:32Z

pcdscalc/be_lens_calcs.py

+        err_msg = ('Provided invalid path for lens set file: %s',
+                   lens_file_path)
+        logger.error(err_msg)
+        raise FileNotFoundError(err_msg)


The error message here is not substituted correctly, it comes out as a tuple

Ohh i see... interesting, thanks for catching this.

Before I commit things though, would something like this be acceptable:

err_msg = f'Provided invalid path for lens set file: {lens_file_path}' logger.error(err_msg) raise FileNotFoundError(err_msg)

I know we don't like using the f-Strings for logging.

Yes, personally I've given up on doing the python logging "properly" with the %s substitutions because I find myself wanting to re-use the strings... I'm fine with f-strings and with .format

My understanding is that when you do logger.error('Bad thing: %s', bad_thing) and it happens many times you can filter on Bad thing: %s to catch all logging messages like this... but unless the log message happens a lot I think this is worth overlooking for formatting clarity/consistency.

@klauer might have an opinion too

An opinion? Of course!

Use:

logging.error('%s', value) if the string isn't going to be reused (generally speaking, I think this is the majority of the cases)

f'{value}' for pretty much everything else.

.format() most of the time ends up not being as nice as an f-string, as it requires you to visually match up the arguments instead of seeing how they're going to be interpolated directly in the string. If you find yourself justifying the use of .format() over an f-string because you need to do some calculation/cleanup before formatting, I'd recommend just making a temporary variable and using an f-string with that.

Thank you, this should be fixed in ef27fed

pcdscalc/be_lens_calcs.py

ZLLentz · 2020-11-02T19:31:05Z

pcdscalc/be_lens_calcs.py

+    beam_size_unfocused : float
+        Radial size of x-ray beam before focusing in meters
+    size_horiz
+    size_vert


These two are missing parameter info and their names don't match the signature

I honestly don't know what these two fields are... I was thinking that they could be the Field of View horizontal, Field of View Vertical - which are just numbers, in meters or millimeter

But looking at the code, the np.max is taking size_horizontal as the a - array like, and the size_vertical as the axis - so this is confusing (unless they meant to use np.maximum?? - but don't think so)

if size_vertical is not None: size_calc_1 = np.max(size_horizontal, size_vertical) else: size_calc_1 = size_horizontal

That's why I had it empty for now until I get more feedback from the scientists.... unless you guys know what they are supposed to be?

All i can say for now i guess is that the way we use it, we can conclude that:

size_horizontal : array_like size_vertical : int or tuple of ints

Fair enough! Thanks for looking into the types

ZLLentz · 2020-11-02T19:33:17Z

I've submitted my last few nitpicks- I'd be happy to merge/tag

ZLLentz · 2020-11-02T23:01:20Z

Ok let's put this to rest

cristinasewell added 4 commits October 20, 2020 13:32

MNT: change from using numpy load and save to json loads and dumps

ec2258e

MNT: add plan_set

ec08482

TST: test lens_transmission

753e195

STY: temporary fix for some docstring

d1f6f96

cristinasewell commented Oct 21, 2020

View reviewed changes

ZLLentz mentioned this pull request Oct 21, 2020

Need to include the standalone calc utils from pcdscalcs pcdshub/hutch-python#240

Closed

TST: test plan_set

64357ac

cristinasewell requested review from klauer, ZLLentz and ZryletTC October 21, 2020 18:38

cristinasewell commented Oct 21, 2020

View reviewed changes

pcdscalc/be_lens_calcs.py Outdated Show resolved Hide resolved

cristinasewell added 2 commits October 21, 2020 12:27

FIX: fix lens_set handling when only one set

8a38673

MNT: add support for IF1 material

1fcd6fb

cristinasewell commented Oct 21, 2020

View reviewed changes

klauer reviewed Oct 21, 2020

View reviewed changes

cristinasewell added 2 commits October 21, 2020 19:33

MNT: raise errors instead of returning when dealing with the file

8d5c474

MNT: handle default density in get_att_len

8f40e53

cristinasewell commented Oct 22, 2020

View reviewed changes

cristinasewell added 3 commits October 22, 2020 09:01

MNT: add option to read the entire stack of lens sets

bda7d97

MNT: fixed some little things

0883169

DOC: updated the docs to reflect the changed with the lens_file

1600b50

cristinasewell mentioned this pull request Oct 23, 2020

Be lens classes pcdshub/pcdsdevices#633

Merged

7 tasks

cristinasewell added 3 commits October 22, 2020 21:10

DOC: add example to plan_set

da5503e

MNT: change back to logging instead of print

30b27b0

REV: reverting back to using prints - i need those to be printed to s…

ebfc87b

…tdout - maybe later we'll change them to a tabulated form or something

klauer reviewed Oct 26, 2020

View reviewed changes

pcdscalc/be_lens_calcs.py Outdated Show resolved Hide resolved

MNT: go back to logging instead of print for the last time

c0a768d

ZLLentz reviewed Oct 26, 2020

View reviewed changes

pcdscalc/be_lens_calcs.py Show resolved Hide resolved

DOC: add some units info

279f107

cristinasewell marked this pull request as ready for review October 27, 2020 17:34

ZLLentz reviewed Oct 28, 2020

View reviewed changes

pcdscalc/be_lens_calcs.py Outdated Show resolved Hide resolved

ZLLentz reviewed Oct 28, 2020

View reviewed changes

MNT: add more units

1643a5d

MNT: handle mutable object as parameter in plan_set

9d052b1

DOC: added a few more units info

25f9348

ZLLentz reviewed Nov 2, 2020

View reviewed changes

pcdscalc/be_lens_calcs.py Show resolved Hide resolved

ZLLentz reviewed Nov 2, 2020

View reviewed changes

pcdscalc/be_lens_calcs.py Show resolved Hide resolved

ZLLentz reviewed Nov 2, 2020

View reviewed changes

pcdscalc/be_lens_calcs.py Show resolved Hide resolved

ZLLentz reviewed Nov 2, 2020

View reviewed changes

ZLLentz approved these changes Nov 2, 2020

View reviewed changes

MNT: fixing error msgs and other little things

ef27fed

ZLLentz merged commit 35c023d into pcdshub:master Nov 2, 2020

klauer mentioned this pull request Nov 3, 2020

Be Lens Calculations Follow Up #7

Closed

2 tasks

cristinasewell mentioned this pull request Dec 4, 2020

ENH: Add lodcm calcs #9

Merged

		logger.info('\n %s', resstring)


		def lens_transmission(r, fwhm, n=1, energy=None, id="Be", lens_thicknes=None):



		# with good_sets = [False, True, True, False, False, True, False, False, False]
		def test_plan_set():

	>>> set_lens_set_to_file(list_of_sets, ../path/to/lens_set)
	>>> set_lens_set_to_file(list_of_sets, '../path/to/lens_set')



		def plan_set(energy, z_offset, z_range, beam_size_unfocused, size_horizontal,
		size_vertical=None, exclude=[], max_tot_number_of_lenses=25,

	Full width at half maximum unfocused.
	Full width at half maximum unfocused in meters.

Be lens calc follow up #8

Be lens calc follow up #8

Conversation

cristinasewell commented Oct 21, 2020

Motivation and Context

How Has This Been Tested?

Where Has This Been Documented?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristinasewell Oct 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristinasewell Oct 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristinasewell Oct 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZLLentz commented Oct 28, 2020

cristinasewell commented Oct 28, 2020 • edited Loading

ZLLentz commented Oct 28, 2020

klauer commented Oct 28, 2020

cristinasewell commented Oct 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristinasewell Nov 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZLLentz commented Nov 2, 2020

ZLLentz commented Nov 2, 2020

cristinasewell Oct 21, 2020 •

edited

Loading

cristinasewell Oct 21, 2020 •

edited

Loading

cristinasewell Oct 28, 2020 •

edited

Loading

cristinasewell commented Oct 28, 2020 •

edited

Loading

cristinasewell Nov 2, 2020 •

edited

Loading