Adapted the get_mcf() function

[SebastienJoly]

Adapted version of the get_mcf() function to calculate higher-orders of the momentum compaction factor using a polynomial fit. Default behaviour is identical to the original version.

[swhite2401]

I had @lfarv as reviewer, he may have suggestions

[lfarv]

Hello @SebastienJoly, Thanks for contributing ! I fully support @swhite2401's comments:

1. In the default case (fit_order == 1), you must return a float, not an array of length 1
2. In case of nstep < 2*fit_order, I prefer raising an Exception (ValueError is good for that), rather than a Warning or printed message.

[SebastienJoly]

Hello both, Many thanks for the detailed review and pointing out the original behaviour of get_mcf() yields a float and not an array. I overlooked this detail in my implementation. I also think it is more reasonable to create a standalone function rather than modifying get_mcf(). I introduced the standalone function get_momentum_compaction_factor_array() in my last commit. Please let me know if this is fine now. Thank you!

[lfarv]

@SebastienJoly: You could keep a single function while keeping the backward compatibility:

    return alphac[0] if len(alphac)<2 else alphac

Now, about introducing a new function, what do you think, @swhite2401 ?

[TeresiaOlsson]

If I might pitch in, I think keeping a single function is a better choice. Based on my experience of supporting new users it becomes confusing to explain how to do something when there are several functions doing similar things.

It's not super nice to have a function which returns different types, but I think it's easier to understand than why there are two different functions.

[swhite2401]

Fine for me to have a single function, this was just personal taste

[SebastienJoly]

Thanks to all for the discussion. I do not have a strong preference between a standalone function or a function returning different types. I went in the direction of the majority and used @lfarv solution to return a float up to the first order of the momentum compaction factor and an array otherwise.

[swhite2401]

Looks good to me, just the help needs to be corrected, the default is not None

One question, are you going to develop more in pyAT? In which case we may give you write access for the project

[SebastienJoly]

You are correct, I modified the docstring accordingly. If everything goes well I do plan on developing more PyAT! I am currently working on a plotting function to return the longitudinal Hamiltonian representation of a RF bucket for an arbitrary momentum compaction factor order and with harmonic cavities. It can also be used to get the momentum deviation acceptance. I do not know if you have any channel to discuss potential PyAT developments. If not we can also organise a meeting to discuss it in the coming weeks.

[swhite2401]

Very good, I've sent you an invite to give you write access to the repo.

Concerning developments can can talk no problem, I see you are from HZB, are you working with @TeresiaOlsson ? She can give you my email address. Otherwise there is the discussion section in the repo that can used to propose ideas.

[TeresiaOlsson]

@swhite2401 @SebastienJoly started at HZB earlier this month. He is our new responsible for the collective effects for BESSY III :) So now we finally have someone dedicated on working with collective effects. I think a meeting would be useful to discuss where we can contribute. I also have a PhD student who started this month working on collective effects for negative momentum compaction factor. She is currently working in mbtrack2 but eventually I hope that she also would like to contribute to pyAT. I will coordinate with @SebastienJoly to set up the meeting.

[swhite2401]

Great, if you could put @lcarver in the loop that'dd be nice. Thanks.

[lfarv]

@SebastienJoly : a last point: you don't use anymore the keep_lattice argument. If set to True, the tracking assumes that the lattice is unchanged, and this gives a small speed gain by avoiding scanning again the attributes of elements. Here it was set to True on all calls except the 1st one. On the 1st call, it was the user's responsibility to set it according to the previous events.

The question is: do we forget this, at the price of a small loss in speed, or do we try to restore the previous behaviour ?

[SebastienJoly]

@SebastienJoly : a last point: you don't use anymore the keep_lattice argument. If set to True, the tracking assumes that the lattice is unchanged, and this gives a small speed gain by avoiding scanning again the attributes of elements. Here it was set to True on all calls except the 1st one. On the 1st call, it was the user's responsibility to set it according to the previous events.

The question is: do we forget this, at the price of a small loss in speed, or do we try to restore the previous behaviour ?

Thank you for the clarification around the keep_lattice argument. Actually, the function is already fast and a small speed gain would be negligible. I set all the 'keep_lattice' arguments to the value in the function argument. I had a bug in one of my scripts last week because of this specific detail. It is better to assume the lattice may have changed.

[swhite2401]

I set all the 'keep_lattice' arguments to the value in the function argument.

You may as well hide it in **kwargs, same as DPStep, up to you. Other than that all good for me, this is ready for merge.

[SebastienJoly]

I would rather explicitly leave these arguments as they help with the readability of the function in my opinion.

[swhite2401]

Ok to merge? Should I re-approve?

[SebastienJoly]

Yes please!