atcollab / at

Accelerator Toolbox
Apache License 2.0
48 stars 31 forks source link

regex option #601

Closed swhite2401 closed 1 year ago

swhite2401 commented 1 year ago

This PR provides a simple switch to activate REGEX in string pattern search as requested in #598 . The default behavior is unchanged.

Deprecated function get_refpts and get_cells have not been modified. For the moment only a local flag is provided as advised in #598.

lfarv commented 1 year ago

For me, upgrading the "low level" selection functions (those in utils.py) is good enough for the moment. For all other functions with a "refpts" argument, and without the "regex" keyword (like linopt6 in my example),

However I would also upgrade get_s_pos in utils.py

swhite2401 commented 1 year ago

I would just need to change lattice pass to propagate this to upper layer functions right? This is not a big change... why not integrate it here while we are at it?

swhite2401 commented 1 year ago

Ah no I take it back, changes are required in many places to integrate this in upper layer function. I will not do it now, like this is good enough.

simoneliuzzo commented 1 year ago

Dear @swhite2401, @lfarv and @ZeusMarti ,

get_refpts and get_cells are exactly the functions that I would like to use. It would be nice if the modification takes them in account.

thank you best regards Simone

oscarxblanco commented 1 year ago

Dear all, in my opinion adding a regular expression package that is compatible with most unix systems is a welcomed addition. This would reduce my wasted time trying to rewrite expressions from other scripts outside python.

Also, I am glad that modifications are kept locally. It is really inconvenient to keep track of the users environment in order to reproduce the same results when having possible differences in global flags.

With respect to the title 'regular expression instead of fnmatch ?' my answer would be no. Regular expressions should not replace fnmatch, but they should be added. For many users the native python functions would be enough.

I have had a look on the code, but, didn't run it or checked. I was gladly mentioned in this PR, but, I'm not assigned to review it. Was it maybe a message to wait for @ZeusMarti ?

swhite2401 commented 1 year ago

@simoneliuzzo, it is ok for me to make the changes so you can use the old functions but these are supposed to be deprecated and you are encouraged to use get_uint32_index instead of get_refpts and get_bool_index instead of get_cells.

lfarv commented 1 year ago

get_refpts and get_cells are exactly the functions that I would like to use. It would be nice if the modification takes them in account

No problem for me ! I just thought that it would be a good opportunity for you to move to the new namings ;-)

swhite2401 commented 1 year ago

I have update the deprecated functions as requested by @simoneliuzzo ok to merge?

swhite2401 commented 1 year ago

Just a side comment for the future:

I agree that we should keep deprecated function alive for backward compatibility. However, we should not request that new functionalities are systematically integrated in these obsolete functions because this mutliplies the work and pyAT will get completely out of hand with multiple functions doing exactly the same thing.

My feeling is that either we accept live with the old functions and their historical names and stop introducing new names and functions but this is a clear limitation for new developments or we clearly state that deprecated function will not be updated which would be my preferred option.

lfarv commented 1 year ago

My feeling is that either we accept live with the old functions and their historical names and stop introducing new names and functions but this is a clear limitation for new developments or we clearly state that deprecated function will not be updated which would be my preferred option.

I fully agree !

swhite2401 commented 1 year ago

I fully agree !

Ok then let's make this clear in future PRs, I will start in #608

swhite2401 commented 1 year ago

@simoneliuzzo ok to merge?

simoneliuzzo commented 1 year ago

I could not test the branch yet. I will try as soon as possible. If @lfarv already tested/approved, this is enough for me.

I find changes of functions names really not helping. To someone trying to learn the code it gives the feeling that the code is definitely not a solid working code, but rather a continuous work in progress. For long term work, a work in progress code is not what I would personally like use. I would then rather chose an other code, guaranteed to work (MAD8, MADX, Elegant, SAD,...)

Change name == Loss of confidence == Less users

If functions are deprecated, it should be announced in a release with a tag name giving indication of when this functions will not be updated anymore.

In short an example: Release AT3.0: new 6D optics computation, radiation and RF handling. .... Starting from the next release the functions get_cells and getrefpts will be deprecated. Consider using get.... and ... . ...

From release AT3.0 on, get_refpts and get_cells give a deprecation warning.

Once they are deprecated (and announced to be so), then I agree they should not be updated.

My request in this PR to update them is due to the fact that: we discussed in person concerning the "not announced" deprecation of get_cells and get_refpts with no link to code releases. The issue that generated this pull request was from two users of get_refpts and get_cells, and the reply was: other functions will be updated. Evidently both users requesting the regex feature did not realize when writing their code that get_cells and get_repts were deprecated.