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

Matlab ring properties #443

Merged
merged 15 commits into from
Sep 5, 2022
Merged

Matlab ring properties #443

merged 15 commits into from
Sep 5, 2022

Conversation

lfarv
Copy link
Contributor

@lfarv lfarv commented Jul 17, 2022

This is a major update of the handling of ring properties in Matlab.

Ring properties are simulating the properties of a "Lattice" object, though a lattice in Matlab is not a class object, for performance reasons. Instead, the ring properties are accessed through the atSetRingProperties and atGetRingProperties functions, and are stored, for some of them, in a RingParam element.

The update of these two functions includes:

  • a new interface for the atGetRingProperties function,
  • the support of many new properties,
  • the full compatibility of property names with PyAT,
  • improved performance.

New interface for atGetRingProperties

The present interface stays unchanged:

>> props = atGetRingProperties(ring)
props = 
  struct with fields:

            FamName: 'S28d'
             Energy: 6.0000e+09
        Periodicity: 32
           Particle: [1×1 atparticle]
             cavpts: 2
    cell_harmnumber: 31
         HarmNumber: 992

The new interface is more flexible, allowing to select the desired properties and giving access to the new ones:
[v1, v2, ...] = atGetRingProperties(ring, 'prop1, 'prop2', ...]
Example:

>> [energy, hh] = atGetRingProperties(ring, 'Energy', 'HarmNumber')
energy =
   6.0000e+09
hh =
   992

The efficiency has been improved, specially if the RingParam element is missing, by caching and reusing previous results. For the best efficiency, it's better to ask for all the desired properties in one call: since some properties depend on others, it avoids repeating the computations.

New properties

Most of the properties available in python are now available. The present list is:

Standard properties:
 'FamName'               Name of the lattice - read/write
 'Energy'                Ring energy [eV] - read/write
 'Periodicity'           Number of periods to build a full ring - read/write
 'Particle'              particle (Particle object) - read/write
 'cavpts'                Location of the main cavities - read/write
 'beta'                  Relativistic beta of the particles - read only
 'gamma'                 Relativistic gamma of the particles - read only
 'rf_frequency'          RF frequency (main cavities) [Hz] - read/write
 'rf_timelag'            RF timelag (main cavities) [m] - read/write
 'BRho'                  Particle rigidity [T.m] - read only
 'mcf'                   Momentum compaction factor "alpha" - read only
 'slip_factor'           Slip factor "eta" - read only

Properties for the full ring ('periods' x cells)
 'HarmNumber'            Harmonic number (cell_harmnumber * Periodicity) - read/write
 'Circumference'         Ring circumference [m] (cell_length * Periodicity) - read only
 'rf_voltage'            RF voltage [V] (cell_rf_voltage * Periodicity) - read/write
 'revolution_frequency'  Revolution frequency [Hz] (cell_revolution_frequency / Periodicity) - read only

Properties for one cell
 'cell_harmnumber'       Harmonic number (cell) - read/write
 'cell_length'           Cell length [m] - read only
 'cell_rf_voltage'       RF voltage [V] (main cavities) - read/write
 'cell_revolution_frequency' Revolution frequency [Hz] (cell) - read only

Custom properties may be added with atSetRingProperties

It also appeared that there might be some confusion between values concerning the single cell and values concerning the full ring ('periodicity' x cells). Historical values still concern the full ring, but the values for one cell are now explicitly identified by a cell_ prefix.

Compatibility with python properties

Aliases are set so that the python property names are correctly interpreted:

historical Matlab name python name
 FamName  name
 Energy  energy
 Periodicity  periodicity
 HarmNumber  harmonic_number
 Circumference  circumference

Both property names may now be used.

@lfarv lfarv added enhancement Matlab For Matlab/Octave AT code labels Jul 17, 2022
@lfarv lfarv marked this pull request as draft July 17, 2022 14:38
@simoneliuzzo
Copy link
Contributor

Dear @lfarv,

I will test this pull request at the end of this week or (more likely) in August.

best regards
Simone

@lnadolski
Copy link
Contributor

lnadolski commented Jul 18, 2022

Hi @lfarv

Can you guide me to use pyat within matlab?
I have add the correction version of python with the command
pyenv('Version', '/Users/nadolski/opt/anaconda3/envs/spyder/lib/python3.9')

How to import the pyat module?
My goal is to test atwritepy

Best regards,

Laurent.

@lnadolski
Copy link
Contributor

Hi @lfarv

Can you guide me to use pyat within matlab? I have add the correction version of python with the command pyenv('Version', '/Users/nadolski/opt/anaconda3/envs/spyder/lib/python3.9')

How to import the pyat module? My goal is to test atwritepy

Best regards,

Laurent.

Finally I got it
insert(py.sys.path, int32(0), '/Users/nadolski/SynologyDrive/Drive/Python/at/pyat')

I am not sure this is documented somewhere.

@lnadolski
Copy link
Contributor

in file githubsetup, I got an error.
Why defining the pyenv if it is allready done.
I suggest testing if defined. If yes, do nothing.

Error using pyenv
Python is loaded. The environment cannot be changed in this MATLAB session. To change the environment, restart MATLAB,
and then call 'pyenv'.
Error in githubsetup (line 16)
pyenv("Version", execfile,'ExecutionMode', execmode);

@lnadolski
Copy link
Contributor

lnadolski commented Jul 18, 2022

Dear @lfarv

I got many errors using githubrun on MATLAB 2021b
but on MATLAB 2022a, it works smoothly

I am very sorry for the bad formatting
`
githubrun
Running pytests
.......... ....

Verification failed in pytests/tunechrom6(lat2=spear3,dp=value3).
---------------------
Framework Diagnostic:
---------------------
verifyEqual failed.
--> The numeric values are not equal using "isequaln".
--> The error was not within absolute tolerance.
--> Failure table:
Index Actual Expected Error RelativeError AbsoluteTolerance
_____ _________________ _________________ ____________________ _____________________ _________________
1 0.313148498418558 0.313311733639992 -0.00016323522143441 -0.000520999387855591 0.0001

Actual Value:
   0.313148498418558   1.349848450150102  -0.010128326571827
Expected Value:
   0.313311733639992   1.349847907885916  -0.010132530748695
------------------
Stack Information:
------------------
In /Users/nadolski/SynologyDrive/Drive/MATLAB/acceleratortoolbox/atmat/attests/pytests.m (pytests.tunechrom6) at 144

================================================================================
....

Error occurred in pytests/radiation_integrals(lat=hmba) and it did not run to completion.
---------
Error ID:
---------
'MATLAB:invalidConversion'
--------------
Error Details:
--------------
Error using double
Conversion to double from py.tuple is not possible.
Error in pytests/radiation_integrals (line 172)
pintegrals=double(lattice.p.get_radiation_integrals());

.

Error occurred in pytests/radiation_integrals(lat=dba) and it did not run to completion.
---------
Error ID:
---------
'MATLAB:invalidConversion'
--------------
Error Details:
--------------
Error using double
Conversion to double from py.tuple is not possible.
Error in pytests/radiation_integrals (line 172)
pintegrals=double(lattice.p.get_radiation_integrals());

.

Error occurred in pytests/radiation_integrals(lat=spear3) and it did not run to completion.
---------
Error ID:
---------
'MATLAB:invalidConversion'
--------------
Error Details:
--------------
Error using double
Conversion to double from py.tuple is not possible.
Error in pytests/radiation_integrals (line 172)
pintegrals=double(lattice.p.get_radiation_integrals());

.......... .......... ..........
.......
Done pytests


Failure Summary:

 Name                                       Failed  Incomplete  Reason(s)
========================================================================================
 pytests/tunechrom6(lat2=spear3,dp=value3)    X                 Failed by verification.
----------------------------------------------------------------------------------------
 pytests/radiation_integrals(lat=hmba)        X         X       Errored.
----------------------------------------------------------------------------------------
 pytests/radiation_integrals(lat=dba)         X         X       Errored.
----------------------------------------------------------------------------------------
 pytests/radiation_integrals(lat=spear3)      X         X       Errored.

Error using matlab.unittest.internal.BaseTestResult/assertSuccess (line 125)
At least one test failed in the test session.
Error in githubrun (line 1)
v=assertSuccess(run(testsuite('atmat/attests'))); `

@lnadolski
Copy link
Contributor

Dear @lfarv

radiation property is missing with respect to python version.
Could we and also a cavity property: 0 → IdentityPass 1 → otherwise ?

@lnadolski
Copy link
Contributor

Dear @lfarv

atmaincavities is missing the search for the minimum frequency.

Copy link
Contributor

@lnadolski lnadolski left a comment

Choose a reason for hiding this comment

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

See my comments in Conversation

@lfarv
Copy link
Contributor Author

lfarv commented Jul 18, 2022

Hello @lnadolski

I appreciate to see your involvement in the evolution of AT ! Thanks for testing and proposing !
I agree, PyAT under Matlab is not (yet) documented. And as you noticed, the support of python in Matlab is evolving in each version. But I'll try to answer some of your questions.

Can you guide me to use pyat within matlab? I have add the correction version of python with the command pyenv('Version', '/Users/nadolski/opt/anaconda3/envs/spyder/lib/python3.9')
How to import the pyat module? My goal is to test atwritepy
Best regards,
Laurent.

Finally I got it insert(py.sys.path, int32(0), '/Users/nadolski/SynologyDrive/Drive/Python/at/pyat')

I am not sure this is documented somewhere.

That's too tricky and dangerous ! All you have to do is to set in pyenv a python version where PyAT is installed. If you installed AT in a python virtual environment, as it is recommended, just put <virtualenv>/bin/python as the python executable in pyenv. Nothing else to do.

in file githubsetup, I got an error.
Why defining the pyenv if it is allready done.
I suggest testing if defined. If yes, do nothing.

As the name suggests, githubrun and githubsetup are intended to be used only in GitHub actions, not by users. I'll add this in their help section. To run the tests, you just need to type:

>> ok=assertSuccess(run(testsuite('atmat/attests')))

But anyway, you apparently succeeded in running them ! I'm discovering the Matlab test framework. It looks powerful, but is not obvious to use. The tests are still preliminary, they may be more systematic.

I got many errors using githubrun on MATLAB 2021b
but on MATLAB 2022a, it works smoothly

Yes, 2022a is much better in converting Matlab and python datatypes, so I focused on this version. Github is able to run this one, and for each previous version, the code needs becomes more and more cluttered with explicit conversions. Do you think going backwards is worth the effort?

radiation property is missing with respect to python version.

Ok. But since in Matlab a lattice is not a well controlled class object, for checking radiation on/off, the check_radiation function scans the whole lattice, rather than checking a flag as in python. So that checking takes almost as long as changing to the desired state… But apart from this performance problem, yes, I can add the property.

Could we and also a cavity property: 0 → IdentityPass 1 → otherwise ?

Yes, how could we call it? Unfortunately, the radiation property is not correctly named: it means "radiation or cavity". A better name would be "longitudinal_motion", but it's too late to change that. Now I would be more prudent in naming this new one.

@lfarv
Copy link
Contributor Author

lfarv commented Jul 18, 2022

Looking at you result:

Index Actual Expected Error RelativeError AbsoluteTolerance
_____ _________________ _________________ ____________________ _____________________ _________________
1 0.313148498418558 0.313311733639992 -0.00016323522143441 -0.000520999387855591 0.0001

Actual Value:
   0.313148498418558   1.349848450150102  -0.010128326571827
Expected Value:
   0.313311733639992   1.349847907885916  -0.010132530748695

There is something strange with this test. It looks like something is not reproducible. I'm looking at that

@lnadolski
Copy link
Contributor

Hello @lnadolski

That's too tricky and dangerous ! All you have to do is to set in pyenv a python version where PyAT is installed. If you installed AT in a python virtual environment, as it is recommended, just put <virtualenv>/bin/python as the python executable in pyenv. Nothing else to do.

OK. but matlab is not running in the virtual environment. Should I do it as well ?

in file githubsetup, I got an error.
Why defining the pyenv if it is allready done.
I suggest testing if defined. If yes, do nothing.

As the name suggests, githubrun and githubsetup are intended to be used only in GitHub actions, not by users. I'll add this in their help section. To run the tests, you just need to type:

>> ok=assertSuccess(run(testsuite('atmat/attests')))

Ok, thanks

But anyway, you apparently succeeded in running them ! I'm discovering the Matlab test framework. It looks powerful, but is not obvious to use. The tests are still preliminary, they may be more systematic.

I got many errors using githubrun on MATLAB 2021b
but on MATLAB 2022a, it works smoothly

Yes, 2022a is much better in converting Matlab and python datatypes, so I focused on this version. Github is able to run this one, and for each previous version, the code needs becomes more and more cluttered with explicit conversions. Do you think going backwards is worth the effort?

This is indeed a concern. On my side, in our development environment and in the control room we have not switched yey to the latest version of matlab. This can take really a few years to do it since I need to convince my control people and get an Linux flavor compatible with the latest version of MATLAB without breaking TANGO. This is really a huge piece of work.

I think we will be several facilities in the same case.
Do you have any workaround to suggest ?

radiation property is missing with respect to python version.

Ok. But since in Matlab a lattice is not a well controlled class object, for checking radiation on/off, the check_radiation function scans the whole lattice, rather than checking a flag as in python. So that checking takes almost as long as changing to the desired state… But apart from this performance problem, yes, I can add the property.

Could we and also a cavity property: 0 → IdentityPass 1 → otherwise ?

Yes, how could we call it? Unfortunately, the radiation property is not correctly named: it means "radiation or cavity". A better name would be "longitudinal_motion", but it's too late to change that. Now I would be more prudent in naming this new one.

I let you choose

;-)

Laurent

@lfarv
Copy link
Contributor Author

lfarv commented Jul 19, 2022

OK. but matlab is not running in the virtual environment. Should I do it as well ?

Matlab is not concerned. Virtual environments are a way to have on the same machine different python "installations", each with its own set of installed software. You can select the wone you want in to ways:

  • using $ source <virtualenv>/bin/activate: this modifies you Unix path to have the selected one accessible by just typing $ python,
  • Without that, by simply by explicitly calling the desired python installation with its full path: $ /<virtualenv>/bin/python. That's what Matlab will do.

@lfarv
Copy link
Contributor Author

lfarv commented Jul 19, 2022

This is indeed a concern. On my side, in our development environment and in the control room we have not switched yey to the latest version of matlab. This can take really a few years to do it since I need to convince my control people and get an Linux flavor compatible with the latest version of MATLAB without breaking TANGO. This is really a huge piece of work.

I see the point. But we are talking here of a single test procedure recently introduced. AT itself is (should be…) compatible with Matlab R2016b, almost 6 years old ! Testing by comparing with python is only possible with the few most recent versions. I'll add a check in the test class to throw an error if using a too old version, with an explicit message. We can go back one or two versions at the cost of less "clean" code, but not more. Which version would you set as the minimum one?

@lnadolski
Copy link
Contributor

This is indeed a concern. On my side, in our development environment and in the control room we have not switched yey to the latest version of matlab. This can take really a few years to do it since I need to convince my control people and get an Linux flavor compatible with the latest version of MATLAB without breaking TANGO. This is really a huge piece of work.

I see the point. But we are talking here of a single test procedure recently introduced. AT itself is (should be…) compatible with Matlab R2016b, almost 6 years old ! Testing by comparing with python is only possible with the few most recent versions. I'll add a check in the test class to throw an error if using a too old version, with an explicit message. We can go back one or two versions at the cost of less "clean" code, but not more. Which version would you set as the minimum one?

I do agree with you. If this affects only the testing procedure, this is fine with me.
At SOLEIL we run Matlab R2018b in operation.

Concerning the scripts using py.at within matlab like atwritepy, we should maybe have a HOWTO to get py.at properly configured as discussed earlier. Otherwise the advice to the user would be to load the m/mat lattice directly from python and do the conversion within python.

To my knowledge only 3 scripts are concerned:
.//atmat/attests/pytests.m
.//atmat/lattice/at2py.m
.//atmat/lattice/atwritepy.m

@lnadolski lnadolski closed this Jul 19, 2022
@lnadolski lnadolski reopened this Jul 19, 2022
@lnadolski
Copy link
Contributor

OK. but matlab is not running in the virtual environment. Should I do it as well ?

Matlab is not concerned. Virtual environments are a way to have on the same machine different python "installations", each with its own set of installed software. You can select the wone you want in to ways:

  • using $ source <virtualenv>/bin/activate: this modifies you Unix path to have the selected one accessible by just typing $ python,
    on my MAC, the command is
    source /bin/activate && conda activate <my_environment>
  • Without that, by simply by explicitly calling the desired python installation with its full path: $ /<virtualenv>/bin/python. That's what Matlab will do.
    Yes indeed

@lfarv
Copy link
Contributor Author

lfarv commented Jul 23, 2022

for @lnadolski: two new properties are available:

'radiation': longitudinal motion (cavities, radiating elements, ...)
'active_cavity': presence of active cavities

@lfarv lfarv marked this pull request as ready for review July 23, 2022 16:09
@lfarv lfarv marked this pull request as draft July 23, 2022 16:09
@lfarv lfarv marked this pull request as ready for review July 23, 2022 16:10
@lfarv
Copy link
Contributor Author

lfarv commented Aug 15, 2022

@simoneliuzzo: can you have a look at this pull request?

@simoneliuzzo
Copy link
Contributor

Dear @lfarv,

I am testing this branch.

I find confusing that output change according to input.

props = atGetRingProperties(betamodel)

[en, hh] = atGetRingProperties(betamodel,'Energy','HarmNumber')

If it is done for speed reasons, I would prefer to have something like:

props = atGetRingProperties(betamodel,'Energy','HarmNumber')

props =

struct with fields:

    Energy: 6.0000e+09
HarmNumber: 992

This is just my personal opinion, not a showstopper.

I continue testing

best regards
Simone

@simoneliuzzo
Copy link
Contributor

Dear @lfarv,

I tried to set Periodicity to 20 and -100. There is nothing preventing these (wrong) actions. The HarmonicNumber changes proportionally to cell_harmnumber

May be there could be some limitations in this input? as for InputParser in matlab some validation function could check that periodicity is allowed (0 < total angle < 2pi for the ring for example).

@simoneliuzzo
Copy link
Contributor

Dear @lfarv ,

I think that the default properties could be all properties.

The simplest call is longer but verbose and answers completely to the question asked by the user when calling the function atGetRingProperties "What are (all) the ring properties?".
To use it in code, the user simply decides which values to use among all, getting the best performance.

@simoneliuzzo
Copy link
Contributor

I have done a few more simple tests. I have errors when I expect errors and changes of parameters as expected.

@simoneliuzzo
Copy link
Contributor

Dear @lfarv

I have tested changing the RF frequency using atSetRingProperty and using atsummary (modified in this branch) to compute the horizontal emittance.

I get the figure below.

Screenshot 2022-08-17 at 11 31 02

Is this expected? I find it personally misleading.

Having compared with real SR data the output of ATX, I consider it to be correct.
On the other hand, a new user, having to chose among atsummary, ringpara, atx, etc... would likely get "confused".

best regards
Simone

@lfarv
Copy link
Contributor Author

lfarv commented Aug 17, 2022

Dear @lfarv,

I am testing this branch.

I find confusing that output change according to input.

@simoneliuzzo : let me show the motivation behind these changes:
The first syntax (props=atGetProperties(ring)) has to be kept or compatibility. It concerns properties stored in the RingParam element: fast access when it exists, so no performance problem. When adding new properties, they could easily have been added to the structure (no compatibility problem), but most of them have to be computed, which leads to a severe performance problem. In real life, you usually need only a few properties at some point, and others at another point. The new input parameters allow that, and it is made such that if a required parameter depends on another one, a single computation occurs.

Concerning the output, the choice between a single structure or multiple output parameters is based on convenience: with multiple parameters, you can choose the variable name you want, you don't need to to remember the field names, and for performance, you avoid the access to a field of a structure: the properties are ready to use !

To summarise, I would say that the old syntax is kept for existing code, but for new code I think that in 99% of cases, the new multiple output syntax is more convenient, and rather intuitive if you look at it: one output per input…

@lfarv
Copy link
Contributor Author

lfarv commented Aug 17, 2022

I tried to set Periodicity to 20 and -100. There is nothing preventing these (wrong) actions. The HarmonicNumber changes proportionally to cell_harmnumber

May be there could be some limitations in this input? as for InputParser in matlab some validation function could check that periodicity is allowed (0 < total angle < 2pi for the ring for example).

Good point. I'll look at that !

@lfarv
Copy link
Contributor Author

lfarv commented Aug 17, 2022

@simoneliuzzo:

I think that the default properties could be all properties.

Do you mean the original syntax props=atGetRIngProperties(ring) should return all the available properties ?
It can be easily done, of course, but I see some drawbacks:

  • existing code using this syntax will suddenly get slower. Computing the momentum compaction factor when you don't need it is not for free…
  • Even worse, it may get slower and slower as new properties will be added.
  • except in "summary" functions, you rarely need all properties together. The idea is on the other hand that you have a fast access to the property you need, and that you can multiply and spread out the calls to the places where they are needed.

If you think that the functionality "What are (all) the ring properties?" is useful, a solution might be something like:

props=atGetRingProperties(ring, 'all')

which does not affect existing code. Is it worth ?

@lfarv
Copy link
Contributor Author

lfarv commented Aug 17, 2022

@simoneliuzzo:

I have tested changing the RF frequency using atSetRingProperty and using atsummary (modified in this branch) to compute the horizontal emittance.

Very interesting. As you know, ringpara and atsummary come from different contributors and are mostly left as they were initially. Looking inside, you'll notice that they use either atlinopt or twissring for optics. And you know that these functions just produce garbage when called with longitudinal motion. So changing the frequency is not a good way to test them.

But I fully agree that your test is legitimate and very instructive. So:

  • The same test run in 4D and varying dp will give you the same information. I'm interested in the results (not sure dp in properly handled).
  • Converting atsummary and ringpara to atlinopt6 is on the top of my todo list. Each function has its fans, so for the moment, I would keep both…
  • since this kind of validation test is out of the scope of this pull request (here we're talking of "property access"), it could be a good topic for an "issue". Feel free to open one, otherwise I'll do it.

And thanks for testing and reporting !

@lfarv lfarv closed this Aug 17, 2022
@lfarv lfarv reopened this Aug 17, 2022
@simoneliuzzo
Copy link
Contributor

@simoneliuzzo:

I think that the default properties could be all properties.

Do you mean the original syntax props=atGetRIngProperties(ring) should return all the available properties ? It can be easily done, of course, but I see some drawbacks:

* existing code using this syntax will suddenly get slower. Computing the momentum compaction factor when you don't need it is not for free…

* Even worse, it may get slower and slower as new properties will be added.

* except in "summary" functions, you rarely need all properties together. The idea is on the other hand that you have a fast access to the property you need, and that you can multiply and spread out the calls to the places where they are needed.

If you think that the functionality "What are (all) the ring properties?" is useful, a solution might be something like:

props=atGetRingProperties(ring, 'all')

which does not affect existing code. Is it worth ?

Dear @lfarv ,

I understand. I think it is an historical naming problem. If I imagine to be a new AT user, I would use atGetRingProperties instead ofringpara atsummary atx as a first guess (and likely the first output of a search in the functions help) to get ring properties. But it is not the purpose of the atGetRingProperties function. If I understand correctly, the purpose is to provide rapidly parameters for efficient code. For example, in my scripts/functions I often call ringpara, atx just to get the emittances, but computing more than just the emittances. Assuming this, to get 'all' parameters, I would use atsummary, atx or ringpara, to get just one or few parameters I would use atGetRingProperties.

May be atGetRingProperty could replace and deprecated ringpara, atsummary and atx?

@simoneliuzzo
Copy link
Contributor

simoneliuzzo commented Aug 18, 2022

Dear @lfarv,
I am testing this branch.
I find confusing that output change according to input.

@simoneliuzzo : let me show the motivation behind these changes: The first syntax (props=atGetProperties(ring)) has to be kept or compatibility. It concerns properties stored in the RingParam element: fast access when it exists, so no performance problem. When adding new properties, they could easily have been added to the structure (no compatibility problem), but most of them have to be computed, which leads to a severe performance problem. In real life, you usually need only a few properties at some point, and others at another point. The new input parameters allow that, and it is made such that if a required parameter depends on another one, a single computation occurs.

Concerning the output, the choice between a single structure or multiple output parameters is based on convenience: with multiple parameters, you can choose the variable name you want, you don't need to to remember the field names, and for performance, you avoid the access to a field of a structure: the properties are ready to use !

To summarise, I would say that the old syntax is kept for existing code, but for new code I think that in 99% of cases, the new multiple output syntax is more convenient, and rather intuitive if you look at it: one output per input…

If the help is explaining this dynamic output I have no practical problems. May be @swhite2401 , @lnadolski and @carmignani have an opinion on this point?

@lnadolski
Copy link
Contributor

@simoneliuzzo:

I think that the default properties could be all properties.

Do you mean the original syntax props=atGetRIngProperties(ring) should return all the available properties ? It can be easily done, of course, but I see some drawbacks:

* existing code using this syntax will suddenly get slower. Computing the momentum compaction factor when you don't need it is not for free…

* Even worse, it may get slower and slower as new properties will be added.

* except in "summary" functions, you rarely need all properties together. The idea is on the other hand that you have a fast access to the property you need, and that you can multiply and spread out the calls to the places where they are needed.

If you think that the functionality "What are (all) the ring properties?" is useful, a solution might be something like:

props=atGetRingProperties(ring, 'all')

which does not affect existing code. Is it worth ?

Dear @lfarv ,

I understand. I think it is an historical naming problem. If I imagine to be a new AT user, I would use atGetRingProperties instead ofringpara atsummary atx as a first guess (and likely the first output of a search in the functions help) to get ring properties. But it is not the purpose of the atGetRingProperties function. If I understand correctly, the purpose is to provide rapidly parameters for efficient code. For example, in my scripts/functions I often call ringpara, atx just to get the emittances, but computing more than just the emittances. Assuming this, to get 'all' parameters, I would use atsummary, atx or ringpara, to get just one or few parameters I would use atGetRingProperties.

May be atGetRingProperty could replace and deprecated ringpara, atsummary and atx?

I have the same understanding as @simoneliuzzo : for all parameters I call either atsummaryor atx. For now atGetRingProperties is practical for calling just a few specific parameters.

Concerning the output format, the solution adopted is fine with me.
I also like the dynamic output format and find it very practical.

@lnadolski
Copy link
Contributor

Dear @lfarv,

  • I would find convenient indeed to get an options to get all parameters as proposed
  • what is the purpose in the help of
  • % 'Energy' Ring energy [eV] % 'energy' " " % 'Periodicity' Number of periods to build a full ring % 'periodicity' " " " " " " " " % 'Particle' particle (Particle object)
    atparamscan is case insensitive anyway.

@lfarv
Copy link
Contributor Author

lfarv commented Aug 18, 2022

@simoneliuzzo, @lnadolski : I think you both summarised the purpose of atGetRingProperties: an efficient access to properties need to implement some computations, rather than a summary of all the ring parameters. As stated in the heading, it is meant to simulate the access to an object properties, though a Matlab lattice is not an object…

I keep from you comments:

  • a validity check on Periodicity,
  • a 'all' flag to force the computation of all available parameters.

@lnadolski : you are right, atGetRingProperties is case independent (it was not initially such), so I'll simplify the help.

case 'brho'
clight=PhysConstant.speed_of_light_in_vacuum.value;
energy=getenergy(ring);
rest_energy=getparticle(ring).rest_energy;
Copy link
Contributor

Choose a reason for hiding this comment

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

@lfarv
Indexing into Function Call Results is only working for R2022a++
this nice feature makes an error for earlier version.

Copy link
Contributor

@lnadolski lnadolski Aug 18, 2022

Choose a reason for hiding this comment

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

same on lines #217 and 228

A quick fix could be

tmp = getparticle(ring);
rest_energy=tmp.rest_energy;

@lfarv
Copy link
Contributor Author

lfarv commented Aug 18, 2022

  • The compatibility with Matlab<R2002a is restored
  • the 'all' flag in atGetRingProperties fills the structure with all available properties
  • to be compatible the naming of things proposed in python, the new property "active_cavity" is rename as "has_cavity". For the same reason "is_6d" is now an alias for "radiation" (less confusing). Of course, "radiation" is still there.

@lfarv
Copy link
Contributor Author

lfarv commented Aug 23, 2022

@simoneliuzzo, @lnadolski : I think all your remarks have been implemented. Anything else before merging?

@lfarv lfarv merged commit 32a8ade into master Sep 5, 2022
@lfarv lfarv deleted the Matlab_properties branch September 5, 2022 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Matlab For Matlab/Octave AT code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants