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

New interface to tracking functions #608

Merged
merged 32 commits into from
Aug 7, 2023
Merged

New interface to tracking functions #608

merged 32 commits into from
Aug 7, 2023

Conversation

swhite2401
Copy link
Contributor

@swhite2401 swhite2401 commented May 31, 2023

This PR proposes to refactor the pyAT tracking functions, presently we have

  • element_pass to track through a single element
  • lattice_pass to track through a sequence of elements
  • patpass to track through a a sequence of elements using python multiprocessing

All these functions are used in internal pyAT basic functionalities such as orbit search or optics calculations.This has several drawbacks:

  • the user is provided with several entry points with different interfaces
  • the interface and performance have to be maintained for internal AT functions preventing any developments or modifications (see lattice_pass modifies initial coordinates #568 for example)

This PR proposes to refactor the tracking function as follows:

  • a single entry point is provided to the users for all tracking functionalities (element, lattice, parallel)
  • the interface is modified to provide flexibility for future developments without affecting existing code
  • protected function are used for pyAT internal use to remove inter-dependency from the user interface

All existing functions remain valid and usable as before but are moved to the deprecated module: they will therefore not be update with future developments

Presently a minimal set of output is provided (some may not be so useful while others could be added) as an example. With the new implementation it is however possible to add outputs or post processing steps without changing the interface or affecting other pyAT functions.

`

@lfarv
Copy link
Contributor

lfarv commented May 31, 2023

@swhite2401
At first glance this looks nice. It will need a more careful investigation, but before that I have a few preliminary remarks:

  • I think one should add @MJGaughran to the reviewers,
  • I would not modify the existing tests because it's the way to be sure that the old functions still work. Or at least keep the most critical, if adding the tests of the new interface make the sequence too heavy,
  • Maybe one could make a minor release after the pending bug fixes are merged, but before this one. This one could be be first of the 1.x series.

@swhite2401
Copy link
Contributor Author

Thanks @lfarv for the feedback, concerning test this was a bit of a dilemma: I do not want to integrate deprecated functions in the test (see discussion in #601). For me it makes no sense, so test should be modified to call 'active' functions, that are in any case called by the old ones...

Then I hesitated between the new interface and the internal_* functions, either one is fine for me. I had the intention to add the test to check that track_function and internal_* give identical results in any case to make sure all functions are tested.

I agree about the release, this also gives time to review this one properly.

@swhite2401 swhite2401 mentioned this pull request May 31, 2023
@lfarv
Copy link
Contributor

lfarv commented May 31, 2023

I do not want to integrate deprecated functions in the test (see discussion in #601)

It's different: we may decide that deprecated functions will not be upgraded any more, but still make sure that they work correctly. New code and access to new functionalities need a switch to the new interface, but existing code must still run ! The tests are the only way to guarantee this !

@swhite2401
Copy link
Contributor Author

The tests are the only way to guarantee this !

Ok, I agree, but then we should provide only minimal testing for old functions: just make sure that results are identical to the new ones.

Otherwise we have to duplicate all tests for all/new functions... this does not make much sense to me since one is calling the other...

pyat/at/tracking/track.py Outdated Show resolved Hide resolved
@swhite2401
Copy link
Contributor Author

I have parametrized all tests to include ALL functions, the overhead is not so large so this fine

pyat/at/tracking/utils.py Outdated Show resolved Hide resolved
pyat/at/tracking/utils.py Outdated Show resolved Hide resolved
@lfarv
Copy link
Contributor

lfarv commented Jul 24, 2023

@swhite2401:
The bug reported above by @oscarxblanco should have been caught by the tests.
But the parametrisation in test_patpass.py does not work !

@swhite2401
Copy link
Contributor Author

I think I have fixed everything, is this ok now?

@lfarv
Copy link
Contributor

lfarv commented Jul 31, 2023

I think I have fixed everything, is this ok now?

I think so !

@oscarxblanco
Copy link
Contributor

@swhite2401 I have run the same test to calculate a diffussion map and it run without errors.

@swhite2401
Copy link
Contributor Author

I am ready to merge, should I wait for another approval?

@lfarv
Copy link
Contributor

lfarv commented Jul 31, 2023

Go on, everything is OK !

pyat/at/tracking/track.py Outdated Show resolved Hide resolved
@lmalina
Copy link

lmalina commented Jul 31, 2023

Following #568 , adding a test that track_function does not modify its input parameters would be nice.

@swhite2401
Copy link
Contributor Author

I have added a test of rin for in_place=False and corrected the docstring as suggested

@swhite2401 swhite2401 requested a review from lfarv July 31, 2023 15:05
@lfarv
Copy link
Contributor

lfarv commented Jul 31, 2023

@swhite2401: it seems that in find_elem_m66, the particle and energy keywords must be forwarded to internal_epass:

internal_epass(elem, in_mat, **kwargs)

@swhite2401
Copy link
Contributor Author

it seems that in find_elem_m66, the particle and energy keywords must be forwarded to internal_epass:

Done

Copy link
Contributor

@lfarv lfarv left a comment

Choose a reason for hiding this comment

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

Once more ! And last one I hope.

@swhite2401
Copy link
Contributor Author

And last one I hope.

I hope as well! I leave up for comments until the end of the week in case

@swhite2401
Copy link
Contributor Author

@lfarv can you re-approve? I had to resolve conflicts with the master...

Copy link
Contributor

@lfarv lfarv left a comment

Choose a reason for hiding this comment

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

Yes, I can approve again !

@swhite2401 swhite2401 merged commit 3403820 into master Aug 7, 2023
31 checks passed
@swhite2401 swhite2401 deleted the tracking_function branch August 7, 2023 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Python For python AT code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants